imp(all): Add general types and utils #15
Conversation
MalteHerrmann
requested review from
Vvaradinov and
GAtom22
and removed request for
a team
|
Important Review skippedAuto reviews are limited to specific labels. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes enhance the Evmos blockchain framework by introducing new utilities, constants, and validation functions. This update focuses on improving address management, gas handling, and transaction indexing, while also refining test coverage for these features. Additionally, the migration to a more structured import system facilitates better code organization and clarity. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Chain
participant IBC
User->>App: Initiates transaction
App->>Chain: Processes transaction
Chain->>IBC: Validates cross-chain denom
Chain-->>App: Returns transaction result
App-->>User: Displays result
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 Configuration File (
|
MalteHerrmann
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (2)
types/indexer.go (1)
11-21: Consider adding more context to the method comments.The comments for the methods are brief. Adding more details about the expected behavior and parameters could improve clarity.
- // LastIndexedBlock returns -1 if indexer db is empty + // LastIndexedBlock returns the last indexed block number. + // Returns -1 if the indexer database is empty. - // GetByTxHash returns nil if tx not found. + // GetByTxHash retrieves a transaction result by its hash. + // Returns nil if the transaction is not found. - // GetByBlockAndIndex returns nil if tx not found. + // GetByBlockAndIndex retrieves a transaction result by the block number and transaction index. + // Returns nil if the transaction is not found.types/hdpath.go (1)
9-15: Consider adding more context to the constant comments.The comments for the constants are brief. Adding more details about their usage and significance could improve clarity.
- // Bip44CoinType satisfies EIP84. See https://github.com/ethereum/EIPs/issues/84 for more info. + // Bip44CoinType is the coin type for Ethereum as per BIP44. + // It satisfies EIP84. See https://github.com/ethereum/EIPs/issues/84 for more info. - // BIP44HDPath is the default BIP44 HD path used on Ethereum. + // BIP44HDPath is the default BIP44 HD path used for Ethereum accounts. + // It follows the standard defined in BIP44.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
types/dynamic_fee.pb.gois excluded by!**/*.pb.gotypes/indexer.pb.gois excluded by!**/*.pb.gotypes/web3.pb.gois excluded by!**/*.pb.go
Files selected for processing (22)
- CHANGELOG.md (1 hunks)
- ethereum/eip712/eip712_test.go (3 hunks)
- ethereum/eip712/encoding.go (1 hunks)
- ethereum/eip712/encoding_legacy.go (1 hunks)
- ethereum/eip712/preprocess.go (1 hunks)
- ethereum/eip712/preprocess_test.go (4 hunks)
- testutil/constants.go (1 hunks)
- types/block.go (1 hunks)
- types/chain_id.go (1 hunks)
- types/chain_id_test.go (1 hunks)
- types/codec.go (1 hunks)
- types/dynamic_fee.go (1 hunks)
- types/errors.go (1 hunks)
- types/gasmeter.go (1 hunks)
- types/hdpath.go (1 hunks)
- types/indexer.go (1 hunks)
- types/int.go (1 hunks)
- types/protocol.go (1 hunks)
- types/validation.go (1 hunks)
- types/validation_test.go (1 hunks)
- utils/utils.go (1 hunks)
- utils/utils_test.go (1 hunks)
Files skipped from review due to trivial changes (4)
- ethereum/eip712/encoding_legacy.go
- ethereum/eip712/preprocess.go
- testutil/constants.go
- types/protocol.go
Additional comments not posted (61)
types/errors.go (2)
1-3: LGTM!The copyright and license information is correctly formatted.
10-14: LGTM!The error handling code is correctly defining a codespace and registering an error.
types/dynamic_fee.go (2)
1-3: LGTM!The copyright and license information is correctly formatted.
9-13: LGTM!The function correctly checks for the dynamic fee extension option and returns a boolean value.
CHANGELOG.md (1)
11-11: LGTM!The changelog entry is correctly formatted and provides a clear description of the changes made in the PR.
types/hdpath.go (1)
21-34: LGTM!The function correctly handles the input parameters and returns the appropriate iterator. Error handling is also properly implemented.
types/int.go (3)
17-24: LGTM! The function is well-implemented.The function correctly checks for overflows and returns appropriate errors.
26-32: LGTM! The function is well-implemented.The function correctly checks for 256-bit overflow and returns appropriate errors.
34-37: LGTM! The function is well-implemented.The function correctly checks if a
big.Intis within 256-bit bounds.types/block.go (1)
11-41: LGTM! The function is well-implemented.The function correctly retrieves the block gas limit from the context and handles different scenarios appropriately.
types/validation.go (4)
13-16: LGTM! The function is well-implemented.The function correctly checks if a hash corresponds to an empty Ethereum hex hash.
18-21: LGTM! The function is well-implemented.The function correctly checks if an address corresponds to an empty Ethereum hex address.
23-32: LGTM! The function is well-implemented.The function correctly validates if a string is a hex formatted Ethereum address and returns appropriate errors.
34-44: LGTM! The function is well-implemented.The function correctly validates if a string is a non-zero hex formatted Ethereum address and returns appropriate errors.
types/chain_id.go (3)
28-35: LGTM! The function implementation is correct.The function correctly validates the chain ID format using the compiled regular expression.
37-57: LGTM! The function implementation is correct.The function correctly parses the chain ID string, handles errors, and converts the epoch to a big integer.
14-26: Verify the regular expression components.The regular expression components seem to be well-defined and combined correctly. Ensure that these components cover all valid chain ID formats.
Verification successful
Verify the regular expression components.
The regular expression components are correctly defined and combined to form the
evmosChainIDregular expression. TheIsValidChainIDfunction appropriately uses this regular expression to validate chain IDs.
types/chain_id.go: Lines 14-26Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the regular expression components cover all valid chain ID formats. # Test: Search for the usage of the regular expressions. Expect: Only valid chain ID formats are matched. rg --type go -A 5 $'regexChainID|regexEIP155Separator|regexEIP155|regexEpochSeparator|regexEpoch'Length of output: 888
Script:
#!/bin/bash # Description: Verify the usage of the `evmosChainID` variable and the `IsValidChainID` function. # Test: Search for the usage of the `evmosChainID` variable. Expect: Appropriate usage in validating chain IDs. rg --type go -A 5 'evmosChainID' # Test: Search for the usage of the `IsValidChainID` function. Expect: Appropriate usage in validating chain IDs. rg --type go -A 5 'IsValidChainID'Length of output: 1846
types/chain_id_test.go (1)
11-87: LGTM! The test cases are well-defined and cover a wide range of scenarios.The test cases correctly test various valid and invalid chain ID formats, edge cases, and error handling.
Verification successful
To verify the coverage of the
ParseChainIDfunction without using thegocommand, we can search for the function definition and its usage across the test files. This will help us determine if the function is adequately tested.
LGTM! The test cases are well-defined and cover a wide range of scenarios.
The test cases correctly test various valid and invalid chain ID formats, edge cases, and error handling. The
ParseChainIDfunction is adequately tested intypes/chain_id_test.go.
types/chain_id_test.go: Test cases forParseChainID.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the coverage of the test cases. # Test: Run the tests and check the coverage. Expect: High coverage for the `ParseChainID` function. go test -cover ./typesLength of output: 94
Script:
#!/bin/bash # Find the definition of the `ParseChainID` function. ast-grep --lang go --pattern 'func ParseChainID($_) $_' # Find all test files that call the `ParseChainID` function. rg 'ParseChainID' --glob '*_test.go' -A 5Length of output: 1777
types/validation_test.go (5)
12-33: LGTM! The test cases are well-defined and cover multiple scenarios.The test cases correctly test various scenarios for the
IsEmptyHashfunction.
35-56: LGTM! The test cases are well-defined and cover multiple scenarios.The test cases correctly test various scenarios for the
IsZeroAddressfunction.
58-87: LGTM! The test cases are well-defined and cover multiple scenarios.The test cases correctly test various scenarios for the
ValidateAddressfunction.
89-118: LGTM! The test cases are well-defined and cover multiple scenarios.The test cases correctly test various scenarios for the
ValidateNonZeroAddressfunction.
120-144: LGTM! The test cases are well-defined and cover multiple scenarios.The test cases correctly test various scenarios for the
SafeInt64function.types/gasmeter.go (12)
12-17: LGTM!The
ErrorNegativeGasConsumedstruct is well-defined and straightforward.
19-23: LGTM!The
ErrorGasOverflowstruct is well-defined and straightforward.
30-36: LGTM!The
NewInfiniteGasMeterWithLimitfunction is well-defined and initializes theinfiniteGasMeterWithLimitcorrectly.
38-41: LGTM!The
GasConsumedfunction correctly returns the consumed gas.
43-49: LGTM!The
GasConsumedToLimitfunction correctly returns the consumed gas.
51-54: LGTM!The
Limitfunction correctly returns the gas limit.
56-64: LGTM!The
addUint64Overflowfunction correctly handles overflow checking.
76-88: LGTM!The
RefundGasfunction correctly handles gas refunding and negative gas consumption checking.
90-93: LGTM!The
IsPastLimitfunction always returns false, which might be intentional for this type of gas meter.
95-98: LGTM!The
IsOutOfGasfunction always returns false, which might be intentional for this type of gas meter.
100-103: LGTM!The
Stringfunction correctly formats and returns the string representation of the gas meter.
105-108: LGTM!The
GasRemainingfunction correctly returns the maximum uint64 value, indicating an infinite gas meter.ethereum/eip712/preprocess_test.go (5)
22-23: LGTM!The new import statements for
testutilandtypesare necessary for the changes made in the test cases.
29-29: LGTM!The
chainIDvariable is correctly updated to usetestutil.ExampleChainID.
98-98: LGTM!The fee denomination is correctly updated to use
testutil.ExampleAttoDenom.
196-198: LGTM!The fee denomination is correctly updated to use
testutil.ExampleAttoDenom.
208-210: LGTM!The fee denomination is correctly updated to use
testutil.ExampleAttoDenom.utils/utils.go (11)
23-27: LGTM!The
EthHexToCosmosAddrfunction correctly converts a Hex string to a Cosmos SDK account address.
29-32: LGTM!The
EthToCosmosAddrfunction correctly converts an Ethereum style address to an SDK address.
34-43: LGTM!The
Bech32ToHexAddrfunction correctly converts a Bech32 address string to an Ethereum address.
45-49: LGTM!The
CosmosToEthAddrfunction correctly converts an SDK account address to an Ethereum address.
51-77: LGTM!The
IsSupportedKeyfunction correctly checks the supported public key types and handles nested multisigs.
79-101: LGTM!The
GetAccAddressFromBech32function correctly converts the Bech32 address to an SDK account address and handles invalid addresses.
103-120: LGTM!The
CreateAccAddressFromBech32function correctly creates the SDK account address from the Bech32 string and handles empty and invalid addresses.
122-139: LGTM!The
GetIBCDenomAddressfunction correctly returns the address from the hash of the ICS20's DenomTrace Path and handles invalid IBC denominations.
141-153: LGTM!The
ComputeIBCDenomTracefunction correctly computes the IBC voucher denom trace.
155-162: LGTM!The
ComputeIBCDenomfunction correctly computes the IBC voucher denom.
164-169: LGTM!The
SortSlicefunction correctly sorts the slice using the generic type constraint.ethereum/eip712/encoding.go (1)
15-15: Verify the impact of the changed import path.The import path for
evmostypeshas been updated fromgithub.com/evmos/evmos/v19/typestogithub.com/evmos/os/types. Ensure that the new import path provides the necessary types and functionalities required by this file.Verification successful
The
ParseChainIDfunction is correctly defined in the new import path.The
ParseChainIDfunction is found in thetypes/chain_id.gofile, which aligns with the new import pathgithub.com/evmos/os/types. This confirms that the new import path provides the necessary types and functionalities required by the file.
- Location of
ParseChainIDfunction:types/chain_id.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new import path provides the necessary types and functionalities. # Test: Search for the usage of `evmostypes` in this file. Expect: No unresolved references. rg --type go 'evmostypes' ethereum/eip712/encoding.goLength of output: 210
Script:
#!/bin/bash # Search for the definition of the ParseChainID function in the new import path ast-grep --lang go --pattern 'func ParseChainID($_) $_' -- github.com/evmos/os/typesLength of output: 159
Script:
#!/bin/bash # Search for the definition of the ParseChainID function across all Go files in the repository ast-grep --lang go --pattern 'func ParseChainID($_) $_'Length of output: 1130
utils/utils_test.go (6)
17-67: LGTM!The test cases for
TestIsSupportedKeyscover a wide range of scenarios and the function is well-structured.
70-126: LGTM!The test cases for
TestGetAccAddressFromBech32cover a wide range of scenarios and the function is well-structured.
129-181: LGTM!The test cases for
TestEvmosCoinDenomcover a wide range of scenarios and the function is well-structured.
184-231: LGTM!The test cases for
TestAccAddressFromBech32cover a wide range of scenarios and the function is well-structured.
234-251: LGTM!The test cases for
TestAddressConversioncover both directions of conversion and ensure correctness. The function is well-structured.
253-297: LGTM!The test cases for
TestGetIBCDenomAddresscover a wide range of scenarios and the function is well-structured.ethereum/eip712/eip712_test.go (3)
31-31: LGTM!The import statement for the
testutilpackage is appropriate for using example values in the test suite.
73-73: LGTM!The assignment of
suite.denomtotestutil.ExampleAttoDenomimproves clarity by using example values from thetestutilpackage.
353-353: LGTM!The assignment of
chainIDtotestutil.ExampleChainIDimproves clarity by using example values from thetestutilpackage.
| // RegisterInterfaces registers the tendermint concrete client-related | ||
| // implementations and interfaces. | ||
| func RegisterInterfaces(registry codectypes.InterfaceRegistry) { | ||
| registry.RegisterImplementations( | ||
| (*authtypes.AccountI)(nil), | ||
| // TODO: uncomment after moving into migrations for EVM version | ||
| // &EthAccount{}, | ||
| ) | ||
| registry.RegisterImplementations( | ||
| (*authtypes.GenesisAccount)(nil), | ||
| // TODO: uncomment after moving into migrations for EVM version | ||
| // &EthAccount{}, | ||
| ) | ||
| registry.RegisterImplementations( | ||
| (*tx.TxExtensionOptionI)(nil), | ||
| &ExtensionOptionsWeb3Tx{}, | ||
| &ExtensionOptionDynamicFeeTx{}, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Reminder: Address TODO comments.
The TODO comments indicate future changes. Ensure these are tracked and addressed in a timely manner.
Do you want me to open a GitHub issue to track these TODO comments?
There was a problem hiding this comment.
sure that would be good, please open one combined for both of the TODOs in this file
There was a problem hiding this comment.
@MalteHerrmann, I've created the GitHub issue to track the TODO comments in the types/codec.go file. You can find the issue here: Track TODO comments in types/codec.go.
Analysis chain
@MalteHerrmann, I'll open a GitHub issue to track these TODO comments.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
gh issue create --title "Track TODO comments in types/codec.go" --body "The file \`types/codec.go\` contains TODO comments that need to be addressed:
- Uncomment the lines related to \`EthAccount\` after moving into migrations for the EVM version.
Relevant TODO comments:
\`\`\`go
// TODO: uncomment after moving into migrations for EVM version
// &EthAccount{},
\`\`\`
File: [types/codec.go](https://github.com/evmos/os/blob/main/types/codec.go)
" --assignee MalteHerrmann --label "enhancement"
Length of output: 525
This PR adds the types and utils from the Evmos repository. Note, that the prior
utils.BaseDenomand chain specific IDs have been changed to be only within the testutils directory, to make it clear for evmOS partners which assumptions are test helpers and which are actually implementation related.Summary by CodeRabbit
New Features
Improvements
Tests