refactor(x/**): rewrite ante handlers as tx validators #20488
Conversation
WalkthroughWalkthroughThe changes involve adding new methods to various structs to support hashing, gas limit, message, and sender retrieval, along with updating import statements and refactoring code to align with new module structures. These updates enhance the functionality and maintainability of transaction handling within the Cosmos SDK. Changes
Sequence Diagram(s) (Beta)Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| @@ -95,6 +98,8 @@ func TestValidateMemo(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestConsumeGasForTxSize(t *testing.T) { | |||
| t.Skip() // TODO(@julienrbrt) Fix after https://github.com/cosmos/cosmos-sdk/pull/20072 | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (5)
contrib/images/simd-env/Dockerfile (5)
Line range hint
3-3: Pin versions in apk add to ensure reproducibility.- RUN apk add build-base git linux-headers + RUN apk add build-base=2.0.1 git=2.30 linux-headers=5.10
Line range hint
3-3: Use the--no-cacheoption withapk addto avoid unnecessary cache storage.- RUN apk add build-base git linux-headers + RUN apk add --no-cache build-base git linux-headers
Line range hint
37-37: Always tag the version of an image explicitly to avoid unexpected changes.- FROM alpine AS run + FROM alpine:3.14 AS run
Line range hint
38-38: Pin versions in apk add to ensure reproducibility.- RUN apk add bash curl jq + RUN apk add bash=5.1 curl=7.76 jq=1.6
Line range hint
38-38: Use the--no-cacheoption withapk addto avoid unnecessary cache storage.- RUN apk add bash curl jq + RUN apk add --no-cache bash curl jq
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (6)
runtime/v2/go.modis excluded by!**/*.modruntime/v2/go.sumis excluded by!**/*.sumx/auth/go.modis excluded by!**/*.modx/auth/go.sumis excluded by!**/*.sumx/tx/go.modis excluded by!**/*.modx/tx/go.sumis excluded by!**/*.sum
Files selected for processing (15)
- contrib/images/simd-env/Dockerfile (1 hunks)
- server/mock/tx.go (2 hunks)
- server/v2/stf/mock/tx.go (1 hunks)
- simapp/ante.go (1 hunks)
- types/mempool/mempool_test.go (3 hunks)
- types/mempool/signer_extraction_adapater_test.go (2 hunks)
- types/tx_msg.go (2 hunks)
- x/auth/ante/ante.go (1 hunks)
- x/auth/ante/basic.go (7 hunks)
- x/auth/ante/basic_test.go (4 hunks)
- x/auth/ante/sigverify.go (1 hunks)
- x/auth/module.go (3 hunks)
- x/auth/tx/config/depinject.go (1 hunks)
- x/auth/tx/gogotx.go (2 hunks)
- x/tx/decode/decode.go (3 hunks)
Files skipped from review due to trivial changes (1)
- x/auth/tx/config/depinject.go
Additional context used
Path-based instructions (13)
simapp/ante.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/mock/tx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/mempool/signer_extraction_adapater_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/auth/ante/ante.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/tx_msg.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/mock/tx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/tx/decode/decode.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/mempool/mempool_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/auth/ante/basic.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/ante/basic_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/auth/tx/gogotx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/ante/sigverify.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Hadolint
contrib/images/simd-env/Dockerfile
[warning] 3-3: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>
[info] 3-3: Use the
--no-cacheswitch to avoid the need to use--updateand remove/var/cache/apk/*when done installing packages
[warning] 37-37: Always tag the version of an image explicitly
[warning] 38-38: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>
[info] 38-38: Use the
--no-cacheswitch to avoid the need to use--updateand remove/var/cache/apk/*when done installing packages
Additional comments not posted (36)
contrib/images/simd-env/Dockerfile (1)
26-30: Ensure that the new modules are necessary and correctly placed in the Dockerfile.simapp/ante.go (1)
41-41: Ensure theNewTxTimeoutHeightDecoratoris correctly integrated and theoptions.Environmentparameter is properly utilized.server/v2/stf/mock/tx.go (1)
26-28: Properly handle nil messages to prevent runtime errors.types/mempool/signer_extraction_adapater_test.go (1)
28-47: Ensure the added methods fornonVerifiableTxare necessary and correctly implemented for testing purposes.x/auth/ante/ante.go (1)
48-48: Ensure theNewTxTimeoutHeightDecoratoris correctly integrated and theoptions.Environmentparameter is properly utilized.types/tx_msg.go (1)
55-56: Ensure thetransaction.Txinterface is correctly embedded in theTxinterface.server/mock/tx.go (4)
98-100: TheHashmethod returns a fixed hash value, which is suitable for mock implementations but should not be used in production.
102-104: TheGetGasLimitmethod returns a fixed gas limit of 0, which is suitable for mock implementations but should not be used in production.
106-108: TheGetMessagesmethod returns nil, which is suitable for mock implementations but should not be used in production.
110-112: TheGetSendersmethod returns nil, which is suitable for mock implementations but should not be used in production.x/tx/decode/decode.go (5)
140-146: TheHashmethod correctly computes the hash based on transaction bytes. Good implementation.
148-153: TheGetGasLimitmethod correctly retrieves the gas limit from the transaction'sAuthInfoand handles potential nil values correctly.
155-166: TheGetMessagesmethod correctly converts protobuf messages to transaction messages and handles potential nil values correctly.
168-172: TheGetSendersmethod correctly retrieves the senders from the transaction and handles potential nil values correctly.
175-180: TheBytesmethod correctly computes the transaction bytes based on the raw transaction data. Good implementation.x/auth/module.go (1)
154-178: TheTxValidatormethod correctly sets up and applies a series of transaction validators. It handles type assertion and error propagation effectively.types/mempool/mempool_test.go (2)
79-97: The methods in thetestTxstruct return fixed values, which are suitable for mock implementations but should not be used in production.
113-131: The methods in thesigErrTxstruct return fixed values, which are suitable for mock implementations but should not be used in production.x/auth/ante/basic.go (4)
35-56: TheValidateTxmethod inValidateBasicDecoratorcorrectly checks the transaction execution mode and validates the basic properties of the transaction. Good implementation.
72-97: TheValidateTxmethod inValidateMemoDecoratorcorrectly validates the memo size against the parameters and handles errors effectively.
Line range hint
119-185: TheValidateTxmethod inConsumeTxSizeGasDecoratorcorrectly consumes gas proportional to the transaction size and simulates gas cost for signatures. Good implementation.
235-263: TheValidateTxmethod inTxTimeoutHeightDecoratorcorrectly checks the transaction timeout height against the current block height and handles errors effectively.x/auth/ante/basic_test.go (6)
4-4: Ensure the new importcontextis utilized appropriately in the tests.
10-11: New imports forappmodule/v2andheaderare added. Verify that these are used effectively in the tests, particularly in the newmockHeaderService.
190-191: ThemockHeaderServiceis introduced to support testing theTxTimeoutHeightDecorator. Ensure that this mock is correctly integrated and used in the tests.
230-231: The methodWithBlockHeightis used to set the block height in themockHeaderService. This is crucial for testing the timeout logic in transactions. Ensure that the method is called correctly and the expected behavior is verified in the tests.
237-249: ThemockHeaderServicestruct and its methods are well-defined. Ensure that theHeaderInfomethod correctly returns the expected header information and that theWithBlockHeightmethod effectively sets the expected block height.
101-102: The test is skipped with a TODO note referencing another PR. Ensure this is addressed before merging to maintain test coverage.Would you like me to help by creating a GitHub issue to track the resolution of this TODO after PR #20072 is merged?
x/auth/tx/gogotx.go (7)
4-4: Ensure the new importcrypto/sha256is used appropriately for cryptographic operations.
11-11: The addition ofgoogle.golang.org/protobuf/protoadaptshould be justified by its usage in the file, particularly for adapting protobuf messages.
16-16: The importcosmossdk.io/core/transactionis crucial for the transaction handling enhancements mentioned in the PR summary.
109-111: The caching mechanism introduced here withcachedHash,cachedBytes, andcachedHashedis a good performance optimization. Ensure thread safety if applicable.
141-146: The methodGetGasLimit()correctly handles potential nil pointers, which is crucial for robust error handling.
148-159: The methodGetMessages()effectively checks for nil values before proceeding, which is a good practice for robust error handling.
161-166: The methodGetSenders()includes necessary nil checks. Ensure that all possible error paths are tested.x/auth/ante/sigverify.go (1)
463-469: TheAnteHandlemethod inValidateSigCountDecoratorcorrectly handles the validation of transaction signatures based on the count. Ensure comprehensive testing for this logic.
x/auth/tx/gogotx.go
Outdated
| func (w *gogoTxWrapper) computeHashAndBytes() { | ||
| bz, err := proto.Marshal(w.decodedTx.TxRaw) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| w.cachedBytes = bz | ||
| w.cachedHash = sha256.Sum256(bz) | ||
| w.cachedHashed = true | ||
| } |
There was a problem hiding this comment.
The computeHashAndBytes method is central to the caching logic. Consider handling errors gracefully instead of using panic.
func (w *gogoTxWrapper) computeHashAndBytes() error {
bz, err := proto.Marshal(w.decodedTx.TxRaw)
if err != nil {
return err
}
w.cachedBytes = bz
w.cachedHash = sha256.Sum256(bz)
w.cachedHashed = true
return nil
}
There was a problem hiding this comment.
@julienrbrt i think we should shift the duty of this towards x/tx.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
x/auth/tx/gogotx.go
Outdated
| func (w *gogoTxWrapper) Hash() [32]byte { | ||
| if !w.cachedHashed { | ||
| w.computeHashAndBytes() | ||
| } | ||
| return w.cachedHash | ||
| } |
There was a problem hiding this comment.
Similar to the Bytes() method, Hash() also benefits from caching. Ensure consistency in error handling as suggested for Bytes().
func (w *gogoTxWrapper) Hash() ([32]byte, error) {
if !w.cachedHashed {
if err := w.computeHashAndBytes(); err != nil {
return [32]byte{}, err
}
}
return w.cachedHash, nil
}
x/auth/tx/gogotx.go
Outdated
| func (w *gogoTxWrapper) Bytes() []byte { | ||
| if !w.cachedHashed { | ||
| w.computeHashAndBytes() | ||
| } | ||
| return w.cachedBytes | ||
| } |
There was a problem hiding this comment.
The method Bytes() utilizes the caching mechanism effectively. However, consider adding error handling for the computeHashAndBytes method in case of failures.
func (w *gogoTxWrapper) Bytes() ([]byte, error) {
if !w.cachedHashed {
if err := w.computeHashAndBytes(); err != nil {
return nil, err
}
}
return w.cachedBytes, nil
}| func (vscd ValidateSigCountDecorator) ValidateTx(ctx context.Context, tx sdk.Tx) error { | ||
| sigTx, ok := tx.(authsigning.SigVerifiableTx) | ||
| if !ok { | ||
| return ctx, errorsmod.Wrap(sdkerrors.ErrTxDecode, "Tx must be a sigTx") | ||
| return errorsmod.Wrap(sdkerrors.ErrTxDecode, "Tx must be a sigTx") | ||
| } | ||
|
|
||
| params := vscd.ak.GetParams(ctx) | ||
| pubKeys, err := sigTx.GetPubKeys() | ||
| if err != nil { | ||
| return ctx, err | ||
| return err | ||
| } | ||
|
|
||
| sigCount := 0 | ||
| for _, pk := range pubKeys { | ||
| sigCount += CountSubKeys(pk) | ||
| if uint64(sigCount) > params.TxSigLimit { | ||
| return ctx, errorsmod.Wrapf(sdkerrors.ErrTooManySignatures, "signatures: %d, limit: %d", sigCount, params.TxSigLimit) | ||
| return errorsmod.Wrapf(sdkerrors.ErrTooManySignatures, "signatures: %d, limit: %d", sigCount, params.TxSigLimit) | ||
| } | ||
| } | ||
|
|
||
| return next(ctx, tx, false) | ||
| return nil |
There was a problem hiding this comment.
The ValidateTx method provides a robust check for the number of signatures against a set limit. Consider adding more detailed error messages to aid in debugging.
func (vscd ValidateSigCountDecorator) ValidateTx(ctx context.Context, tx sdk.Tx) error {
sigTx, ok := tx.(authsigning.SigVerifiableTx)
if !ok {
return errorsmod.Wrap(sdkerrors.ErrTxDecode, "Tx must be a sigTx with signature verification capabilities")
}
params := vscd.ak.GetParams(ctx)
pubKeys, err := sigTx.GetPubKeys()
if err != nil {
return err
}
sigCount := 0
for _, pk := range pubKeys {
sigCount += CountSubKeys(pk)
if uint64(sigCount) > params.TxSigLimit {
return errorsmod.Wrapf(sdkerrors.ErrTooManySignatures, "too many signatures: %d, limit: %d", sigCount, params.TxSigLimit)
}
}
return nil
}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- x/auth/ante/basic.go (7 hunks)
- x/auth/ante/sigverify.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/auth/ante/basic.go
- x/auth/ante/sigverify.go
| @@ -126,3 +136,56 @@ func (d *Decoder) Decode(txBytes []byte) (*DecodedTx, error) { | |||
| Signers: signers, | |||
| }, nil | |||
| } | |||
|
|
|||
| // Hash implements the interface for the Tx interface. | |||
| func (dtx *DecodedTx) Hash() [32]byte { | |||
There was a problem hiding this comment.
These changes were originally in #19588 but I can't convince myself they're needed here since gogotx handles the conversion and tx building. Is it true they're not needed?
There was a problem hiding this comment.
Right, if you are not supposed to use a DecodedTx directly, I guess it makes sense to remove this.
There was a problem hiding this comment.
looks like we should do the reverse: #20488 (comment)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
x/auth/go.modis excluded by!**/*.modx/auth/go.sumis excluded by!**/*.sum
Files selected for processing (5)
- server/mock/tx.go (2 hunks)
- types/mempool/signer_extraction_adapater_test.go (2 hunks)
- types/tx_msg.go (3 hunks)
- x/auth/module.go (3 hunks)
- x/auth/tx/gogotx.go (2 hunks)
Files skipped from review as they are similar to previous changes (5)
- server/mock/tx.go
- types/mempool/signer_extraction_adapater_test.go
- types/tx_msg.go
- x/auth/module.go
- x/auth/tx/gogotx.go
This reverts commit c8adf60.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (6)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sumsimapp/go.modis excluded by!**/*.modsimapp/go.sumis excluded by!**/*.sumtests/go.modis excluded by!**/*.modtests/go.sumis excluded by!**/*.sum
Files selected for processing (5)
- x/auth/tx/builder.go (1 hunks)
- x/auth/tx/encoder.go (2 hunks)
- x/auth/tx/gogotx.go (8 hunks)
- x/auth/tx/legacy_amino_json.go (2 hunks)
- x/tx/decode/decode.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/auth/tx/gogotx.go
- x/tx/decode/decode.go
Additional context used
Path-based instructions (3)
x/auth/tx/encoder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/tx/legacy_amino_json.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/tx/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (4)
x/auth/tx/encoder.go (2)
35-35: LGTM! Ensure that the type assertion forgogoWrapperis always valid in all use cases.Verification successful
The implementation of
DefaultJSONTxEncoderincludes a type assertion forgogoWrapper, and it returns an error if the assertion fails. This ensures that the type assertion is handled correctly within the function itself.
DefaultJSONTxEncoderimplementation:func DefaultJSONTxEncoder(cdc codec.Codec) sdk.TxEncoder { jsonMarshaler := protojson.MarshalOptions{ Indent: "", UseProtoNames: true, UseEnumNumbers: false, } return func(tx sdk.Tx) ([]byte, error) { gogoWrapper, ok := tx.(*gogoTxWrapper) if !ok { return nil, fmt.Errorf("unexpected tx type: %T", tx) } return jsonMarshaler.Marshal(gogoWrapper.Tx) } }Usage in
x/auth/tx/config.go:txConfig.jsonEncoder = DefaultJSONTxEncoder(protoCodec)The type assertion for
gogoWrapperis handled correctly within theDefaultJSONTxEncoderfunction, ensuring that any incorrect type will result in an error.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all uses of `DefaultJSONTxEncoder` handle the type assertion correctly. # Test: Search for the function usage. Expect: Proper handling of type assertion. rg --type go $'DefaultJSONTxEncoder'Length of output: 310
Script:
#!/bin/bash # Extract the relevant code from x/auth/tx/config.go to inspect the usage of DefaultJSONTxEncoder. rg --type go --context 10 'DefaultJSONTxEncoder' x/auth/tx/config.goLength of output: 779
Script:
#!/bin/bash # Extract the implementation of DefaultJSONTxEncoder. rg --type go --context 10 'func DefaultJSONTxEncoder' x/auth/tx/encoder.go # Extract the usage of txConfig.jsonEncoder. rg --type go 'txConfig.jsonEncoder' x/auth/tx/Length of output: 838
19-19: LGTM! Ensure that the type assertion forgogoWrapperis always valid in all use cases.Verification successful
The type assertion for
gogoWrapperinDefaultTxEncoderis handled correctly within the function itself, ensuring that any incorrect type is caught and an error is returned. The usages inconfig.goandstdtx_builder.godo not introduce any issues related to this type assertion.
encoder.go: The type assertion is handled within theDefaultTxEncoderfunction.config.go: The function is used without additional type assertion handling, but this is acceptable since the function itself handles it.stdtx_builder.go: This file defines a differentDefaultTxEncoderfunction that does not impact the type assertion handling ofgogoWrapper.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all uses of `DefaultTxEncoder` handle the type assertion correctly. # Test: Search for the function usage. Expect: Proper handling of type assertion. rg --type go $'DefaultTxEncoder'Length of output: 469
Script:
#!/bin/bash # Description: Review the implementation of `DefaultTxEncoder` in `encoder.go`. # Test: Extract the implementation of `DefaultTxEncoder`. ast-grep --lang go --pattern $'func DefaultTxEncoder() sdk.TxEncoder { $$$ }' x/auth/tx/encoder.goLength of output: 566
Script:
#!/bin/bash # Description: Review the usage of `DefaultTxEncoder` in `config.go`. # Test: Extract the relevant lines around the usage of `DefaultTxEncoder`. rg --context 5 'DefaultTxEncoder' x/auth/tx/config.goLength of output: 469
Script:
#!/bin/bash # Description: Review the usage of `DefaultTxEncoder` in `stdtx_builder.go`. # Test: Extract the relevant lines around the usage of `DefaultTxEncoder`. rg --context 5 'DefaultTxEncoder' x/auth/migrations/legacytx/stdtx_builder.goLength of output: 391
x/auth/tx/legacy_amino_json.go (1)
Line range hint
51-72: LGTM! Ensure that the handling of non-critical fields and extension options is consistent across all transaction types.Verification successful
The search results indicate that the
GetSignBytesfunction is implemented in several files, with some implementations being placeholders (i.e.,panic("not implemented")). However, the relevant implementations that handle non-critical fields and extension options are found in the following files:
x/tx/signing/directaux/direct_aux.gox/tx/signing/aminojson/aminojson.gox/auth/tx/legacy_amino_json.goThese implementations show consistent handling of non-critical fields and extension options, aligning with the changes made in
x/auth/tx/legacy_amino_json.go.
x/tx/signing/directaux/direct_aux.go: Handles fee payer and signer data, ensuring proper authorization.x/tx/signing/aminojson/aminojson.go: Checks for unknown fields and unsupported protobuf extension options.x/auth/tx/legacy_amino_json.go: Validates non-critical fields and extension options, ensuring they are not present.Based on this analysis, the handling of non-critical fields and extension options appears consistent across the relevant transaction types.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all transaction types correctly handle non-critical fields and extension options. # Test: Search for the function usage. Expect: Consistent handling across all transaction types. rg --type go $'GetSignBytes'Length of output: 9735
Script:
#!/bin/bash # Description: Extract implementations of GetSignBytes to verify handling of non-critical fields and extension options. # Extract implementations of GetSignBytes from relevant files. ast-grep --lang go --pattern 'func ($_ $_) GetSignBytes($_, $_, $_) ($_ $_) { $$$ }'Length of output: 86
Script:
#!/bin/bash # Description: Extract implementations of GetSignBytes to verify handling of non-critical fields and extension options. # Extract implementations of GetSignBytes with different signatures. ast-grep --lang go --pattern 'func ($_ $_) GetSignBytes($_, $_, $_) ($_ $_) { $$$ }' ast-grep --lang go --pattern 'func ($_ $_) GetSignBytes($_, $_) ($_ $_) { $$$ }' ast-grep --lang go --pattern 'func ($_ $_) GetSignBytes($_) ($_ $_) { $$$ }'Length of output: 248
Script:
#!/bin/bash # Description: Extract implementations of GetSignBytes to verify handling of non-critical fields and extension options. # Search for GetSignBytes function definitions to capture the exact signatures. rg --type go --pcre2 --regexp 'func \(\w+ \w+\) GetSignBytes\('Length of output: 1066
Script:
#!/bin/bash # Description: Extract implementations of GetSignBytes to verify handling of non-critical fields and extension options. # Extract implementations of GetSignBytes from the identified files. ast-grep --lang go --pattern 'func (s directHandler) GetSignBytes($_, $_, $_) ($_ $_) { $$$ }' x/tx/signing/handler_map_test.go ast-grep --lang go --pattern 'func (s aminoJSONHandler) GetSignBytes($_, $_, $_) ($_ $_) { $$$ }' x/tx/signing/handler_map_test.go ast-grep --lang go --pattern 'func (h SignModeHandler) GetSignBytes($_, $_, $_) ($_ $_) { $$$ }' x/tx/signing/directaux/direct_aux.go ast-grep --lang go --pattern 'func (h SignModeHandler) GetSignBytes($_, $_, $_) ($_ $_) { $$$ }' x/tx/signing/aminojson/aminojson.go ast-grep --lang go --pattern 'func (s signModeLegacyAminoJSONHandler) GetSignBytes($_, $_, $_) ($_ $_) { $$$ }' x/auth/tx/legacy_amino_json.go ast-grep --lang go --pattern 'func (s directHandler) GetSignBytes($_, $_, $_) ($_ $_) { $$$ }' x/accounts/depinject.go ast-grep --lang go --pattern 'func (s directHandler) GetSignBytes($_, $_, $_) ($_ $_) { $$$ }' tests/integration/auth/keeper/msg_server_test.goLength of output: 945
Script:
#!/bin/bash # Description: Extract implementations of GetSignBytes to verify handling of non-critical fields and extension options. # Extract implementations of GetSignBytes from the identified files. rg --type go --pcre2 --regexp 'func \(\w+ \w+\) GetSignBytes\(' -A 20Length of output: 9566
x/auth/tx/builder.go (1)
36-40: LGTM! Ensure that the handling of signer information is consistent and error-free across all transaction types.Verification successful
The function
newBuilderFromDecodedTxis used in theWrapTxBuildermethod of theconfigstruct inx/auth/tx/config.go. This method wraps a provided transaction and creates a new builder from the decoded transaction.The handling of signer information appears to be consistent and integrated within the
WrapTxBuildermethod, ensuring that the signer information is correctly processed when wrapping transactions.
x/auth/tx/config.go:
WrapTxBuildermethod usesnewBuilderFromDecodedTxto handle signer information.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all transaction types correctly handle signer information. # Test: Search for the function usage. Expect: Consistent handling across all transaction types. rg --type go $'newBuilderFromDecodedTx'Length of output: 327
Script:
#!/bin/bash # Extract the relevant code from x/auth/tx/config.go to review the handling of signer information. sed -n '1,50p' x/auth/tx/config.goLength of output: 2110
Script:
#!/bin/bash # Extract additional lines from x/auth/tx/config.go to review the handling of signer information. sed -n '51,100p' x/auth/tx/config.goLength of output: 2448
Script:
#!/bin/bash # Extract additional lines from x/auth/tx/config.go to review the handling of signer information. sed -n '101,150p' x/auth/tx/config.goLength of output: 1927
Script:
#!/bin/bash # Extract additional lines from x/auth/tx/config.go to review the handling of signer information. sed -n '151,200p' x/auth/tx/config.goLength of output: 1720
Script:
#!/bin/bash # Extract additional lines from x/auth/tx/config.go to review the handling of signer information. sed -n '201,250p' x/auth/tx/config.goLength of output: 1347
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (8)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sumsimapp/go.modis excluded by!**/*.modsimapp/go.sumis excluded by!**/*.sumtests/go.modis excluded by!**/*.modtests/go.sumis excluded by!**/*.sumx/auth/go.modis excluded by!**/*.modx/auth/go.sumis excluded by!**/*.sum
Files selected for processing (1)
- x/auth/module.go (3 hunks)
Additional context used
Path-based instructions (1)
x/auth/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
golangci-lint
x/auth/module.go
38-38: cannot use AppModule{} (value of type AppModule) as "cosmossdk.io/core/appmodule/v2".HasGenesis value in variable declaration: AppModule does not implement "cosmossdk.io/core/appmodule/v2".HasGenesis (missing method IsOnePerModuleType)
39-39: cannot use AppModule{} (value of type AppModule) as "cosmossdk.io/core/appmodule/v2".AppModule value in variable declaration: AppModule does not implement "cosmossdk.io/core/appmodule/v2".AppModule (missing method IsOnePerModuleType)
40-40: cannot use AppModule{} (value of type AppModule) as "cosmossdk.io/core/appmodule".HasServices value in variable declaration: AppModule does not implement "cosmossdk.io/core/appmodule".HasServices (missing method IsOnePerModuleType)
41-41: cannot use AppModule{} (value of type AppModule) as "cosmossdk.io/core/appmodule/v2".HasMigrations value in variable declaration: AppModule does not implement "cosmossdk.io/core/appmodule/v2".HasMigrations (missing method IsOnePerModuleType)
Additional comments not posted (2)
x/auth/module.go (2)
12-16: Ensure new imports are utilized correctly.The new imports for
appmodulev2,transaction, andanteare correctly placed and are essential for the new functionality introduced in this module.
157-180: Review newTxValidatorfunction for correctness and efficiency.The
TxValidatorfunction correctly implements theappmodulev2.HasTxValidatorinterface. It uses a series of validators to ensure the transaction meets various criteria. The function is well-structured and should operate efficiently under normal conditions.
| _ appmodulev2.HasGenesis = AppModule{} | ||
| _ appmodulev2.AppModule = AppModule{} | ||
| _ appmodule.HasServices = AppModule{} | ||
| _ appmodulev2.HasMigrations = AppModule{} |
There was a problem hiding this comment.
Resolve interface implementation issues.
The AppModule struct is declared to implement several interfaces from appmodulev2 and appmodule, but it's missing the required IsOnePerModuleType method for these interfaces. This needs to be implemented to satisfy the interface contracts.
type AppModule struct {
accountKeeper keeper.AccountKeeper
randGenAccountsFn types.RandomGenAccountsFn
accountsModKeeper types.AccountsModKeeper
cdc codec.Codec
+ IsOnePerModuleType() bool {
+ // Implementation here
+ }
}Committable suggestion was skipped due low confidence.
Tools
golangci-lint
38-38: cannot use AppModule{} (value of type AppModule) as "cosmossdk.io/core/appmodule/v2".HasGenesis value in variable declaration: AppModule does not implement "cosmossdk.io/core/appmodule/v2".HasGenesis (missing method IsOnePerModuleType)
39-39: cannot use AppModule{} (value of type AppModule) as "cosmossdk.io/core/appmodule/v2".AppModule value in variable declaration: AppModule does not implement "cosmossdk.io/core/appmodule/v2".AppModule (missing method IsOnePerModuleType)
40-40: cannot use AppModule{} (value of type AppModule) as "cosmossdk.io/core/appmodule".HasServices value in variable declaration: AppModule does not implement "cosmossdk.io/core/appmodule".HasServices (missing method IsOnePerModuleType)
41-41: cannot use AppModule{} (value of type AppModule) as "cosmossdk.io/core/appmodule/v2".HasMigrations value in variable declaration: AppModule does not implement "cosmossdk.io/core/appmodule/v2".HasMigrations (missing method IsOnePerModuleType)
Unfortunately no, as we want to tag core v1, and we don't want to bump core in x/tx just yet. |
Description
Bring tx validators from
server_modularby cherry-picking:Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
TxValidatorfunction to improve transaction validation.Improvements