fix(evm): commit temporary state on precompile start, avoiding bug stemming from uncommitted, dirty journal entries in the EVM StateDB #2086
Conversation
WalkthroughThis pull request includes extensive modifications across various files in the NibiruChain project, focusing on enhancing functionality, error handling, and clarity. Key changes involve restructuring the changelog, updating method signatures to include additional return values, and refining error handling in transaction processing and state management. New utility functions and test cases are introduced, while existing ones are adjusted for improved readability and maintainability. The changes aim to streamline interactions with the Ethereum Virtual Machine (EVM) and enhance the robustness of the codebase. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2086 +/- ##
==========================================
+ Coverage 63.98% 64.48% +0.50%
==========================================
Files 270 270
Lines 21092 21185 +93
==========================================
+ Hits 13495 13662 +167
+ Misses 6660 6573 -87
- Partials 937 950 +13
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (11)
x/evm/evmtest/erc20.go (1)
26-26: Approve the change with a minor suggestion for improvement.The modification enhances the readability of error messages in ERC20 balance assertions, which is beneficial for debugging. However, to maintain efficiency while keeping the improved error message, consider using a hybrid approach:
- assert.Equal(t, expectedBalance.String(), actualBalance.String(), "expected %s, got %s", expectedBalance, actualBalance) + assert.True(t, expectedBalance.Cmp(actualBalance) == 0, "expected %s, got %s", expectedBalance, actualBalance)This change retains the efficient
Cmpmethod for the actual comparison while still providing the readable error message with string representations.x/evm/precompile/funtoken_test.go (2)
129-133: LGTM: Improved contract call simulation.The change to use
evmtest.CallContractTxis a good improvement. It likely provides a more comprehensive simulation of a contract call, including nonce retrieval, transaction message generation, and signing.Consider adding a comment explaining the benefits of using
evmtest.CallContractTxover the directCallContractWithInputfor better code documentation.
140-141: LGTM: Improved balance comparison.The modification to compare string representations of the amounts in the balance assertion is a good change. It ensures consistent type comparison and aligns with updates in other parts of the codebase.
Consider extracting the expected balance "420" into a constant or variable at the beginning of the test function for better readability and easier maintenance.
x/evm/keeper/funtoken_from_erc20_test.go (3)
Line range hint
213-226: LGTM: Consistent terminology updateThe changes from "Cosmos" to "Bank" in the comments and function calls are consistent with the overall refactoring. This improves clarity and accurately represents the module being tested.
Consider adding a brief comment explaining the significance of the
bankSendfunction call to improve test readability.
Line range hint
234-247: LGTM: Good addition of error case testingThe new test case for sending too many tokens improves the overall test coverage by checking error handling. This is a valuable addition to ensure the system behaves correctly under invalid conditions.
Consider enhancing the error check on line 247 to be more specific:
-s.Require().Error(err, evmResp.String()) +s.Require().ErrorContains(err, "insufficient balance", "Expected an insufficient balance error") +s.T().Logf("Error response: %s", evmResp.String())This change would make the test more robust by checking for a specific error message and provide more detailed logging.
Line range hint
268-278: LGTM: Good addition of reverse error case testingThe new test case for sending too many Bank tokens back to ERC20 improves the overall test coverage by checking error handling in the reverse direction. This is a valuable addition to ensure the system behaves correctly under invalid conditions in both conversion directions.
Consider enhancing the error check on line 278 to be more specific and informative:
-s.Require().Error(err) +s.Require().ErrorContains(err, "insufficient balance", "Expected an insufficient balance error when converting more coins than available") +s.T().Logf("Error response: %s", err.Error())This change would make the test more robust by checking for a specific error message and provide more detailed logging, which can be helpful for debugging and understanding test failures.
x/evm/keeper/funtoken_from_coin_test.go (1)
285-285: Approve the change with a minor suggestion for consistency.The change improves readability by directly comparing the balance string to "0". This is clearer than the previous
Cmpmethod usage. However, for consistency withbig.Intcomparisons elsewhere in the codebase, consider usingbalance.Sign() == 0instead. This maintains precision while still being readable.Consider updating the line to:
-s.Require().Equal("0", balance.String()) +s.Require().Zero(balance.Sign(), "Expected zero balance")This change maintains the precision of
big.Intcomparisons while still being clear and readable.x/evm/precompile/oracle.go (1)
Line range hint
75-77: Fix Inconsistent Error Message for Argument CountThe error message in
decomposeQueryExchangeRateArgsincorrectly states that 3 arguments are expected, whereas the function checks for exactly 1 argument. This mismatch can lead to confusion when debugging.Apply this diff to correct the error message:
if len(args) != 1 { - err = fmt.Errorf("expected 3 arguments but got %d", len(args)) + err = fmt.Errorf("expected 1 argument but got %d", len(args)) return }x/evm/statedb/journal.go (1)
165-165: Remove Debug Print StatementThe
fmt.Printf("TODO: UD-DEBUG: PrecompileSnapshotBeforeRun.Revert called\n")statement appears to be a debug print left in the code. It's advisable to remove this to prevent unnecessary console output in production.Apply this diff to remove the debug statement:
- fmt.Printf("TODO: UD-DEBUG: PrecompileSnapshotBeforeRun.Revert called\n")x/evm/evmtest/tx.go (2)
23-24: Organize Import StatementsThe import statements at lines 23-24 could be organized for better readability by grouping them logically (standard library packages, third-party packages, and project-specific packages).
Apply this diff to reorganize the imports:
import ( + "encoding/json" + "fmt" + "math/big" + "testing" + + "github.com/cosmos/cosmos-sdk/crypto/keyring" + sdkmath "cosmossdk.io/math" + + sdk "github.com/cosmos/cosmos-sdk/types" + gethcommon "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/common/hexutil" + gethcore "github.com/ethereum/go-ethereum/core/types" + "github.com/ethereum/go-ethereum/crypto" + gethparams "github.com/ethereum/go-ethereum/params" + "github.com/stretchr/testify/require" + + srvconfig "github.com/NibiruChain/nibiru/v2/app/server/config" + "github.com/NibiruChain/nibiru/v2/x/evm" + "github.com/NibiruChain/nibiru/v2/x/evm/embeds" + "github.com/NibiruChain/nibiru/v2/x/evm/statedb" )
Line range hint
267-301: Improve Documentation and Error Handling inGenerateEthTxMsgAndSignerThe function
GenerateEthTxMsgAndSignercan benefit from enhanced documentation and consistent error handling.
- Update the function comments to match Go documentation conventions.
- Ensure all error returns provide context.
Apply this diff to improve the documentation:
// GenerateEthTxMsgAndSigner estimates gas, sets gas limit, and returns the Ethereum transaction message and signers. // // Usage: // // ```go // evmTxMsg, gethSigner, krSigner, err := GenerateEthTxMsgAndSigner( // jsonTxArgs, deps, sender, // ) // err := evmTxMsg.Sign(gethSigner, sender.KeyringSigner) // ``` func GenerateEthTxMsgAndSigner( jsonTxArgs evm.JsonTxArgs, deps *TestDeps, sender EthPrivKeyAcc, ) (evmTxMsg *evm.MsgEthereumTx, gethSigner gethcore.Signer, krSigner keyring.Signer, err error) { estimateArgs, err := json.Marshal(&jsonTxArgs) if err != nil { return nil, nil, nil, fmt.Errorf("failed to marshal jsonTxArgs: %w", err) } res, err := deps.App.EvmKeeper.EstimateGas( sdk.WrapSDKContext(deps.Ctx), &evm.EthCallRequest{ Args: estimateArgs, GasCap: srvconfig.DefaultEthCallGasLimit, ProposerAddress: []byte{}, ChainId: deps.App.EvmKeeper.EthChainID(deps.Ctx).Int64(), }, ) if err != nil { return nil, nil, nil, fmt.Errorf("failed to estimate gas: %w", err) } jsonTxArgs.Gas = (*hexutil.Uint64)(&res.Gas) evmTxMsg = jsonTxArgs.ToMsgEthTx() gethSigner = gethcore.LatestSignerForChainID(deps.App.EvmKeeper.EthChainID(deps.Ctx)) return evmTxMsg, gethSigner, sender.KeyringSigner, nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 220-222: x/evm/evmtest/tx.go#L220-L222
Added lines #L220 - L222 were not covered by tests
[warning] 225-225: x/evm/evmtest/tx.go#L225
Added line #L225 was not covered by tests
[warning] 237-248: x/evm/evmtest/tx.go#L237-L248
Added lines #L237 - L248 were not covered by tests
[warning] 250-255: x/evm/evmtest/tx.go#L250-L255
Added lines #L250 - L255 were not covered by tests
[warning] 257-261: x/evm/evmtest/tx.go#L257-L261
Added lines #L257 - L261 were not covered by tests
[warning] 263-264: x/evm/evmtest/tx.go#L263-L264
Added lines #L263 - L264 were not covered by tests
[warning] 280-280: x/evm/evmtest/tx.go#L280
Added line #L280 was not covered by tests
[warning] 283-283: x/evm/evmtest/tx.go#L283
Added line #L283 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
- CHANGELOG.md (1 hunks)
- x/evm/evmtest/erc20.go (1 hunks)
- x/evm/evmtest/test_deps.go (2 hunks)
- x/evm/evmtest/tx.go (6 hunks)
- x/evm/keeper/erc20.go (1 hunks)
- x/evm/keeper/funtoken_from_coin_test.go (1 hunks)
- x/evm/keeper/funtoken_from_erc20_test.go (5 hunks)
- x/evm/logs.go (0 hunks)
- x/evm/logs_test.go (1 hunks)
- x/evm/precompile/funtoken.go (3 hunks)
- x/evm/precompile/funtoken_test.go (3 hunks)
- x/evm/precompile/oracle.go (2 hunks)
- x/evm/precompile/precompile.go (3 hunks)
- x/evm/precompile/wasm.go (2 hunks)
- x/evm/statedb/interfaces.go (1 hunks)
- x/evm/statedb/journal.go (8 hunks)
- x/evm/statedb/state_object.go (1 hunks)
- x/evm/statedb/statedb.go (4 hunks)
💤 Files with no reviewable changes (1)
- x/evm/logs.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/evm/evmtest/test_deps.go
[warning] 60-61: x/evm/evmtest/test_deps.go#L60-L61
Added lines #L60 - L61 were not covered by testsx/evm/evmtest/tx.go
[warning] 129-131: x/evm/evmtest/tx.go#L129-L131
Added lines #L129 - L131 were not covered by tests
[warning] 161-161: x/evm/evmtest/tx.go#L161
Added line #L161 was not covered by tests
[warning] 166-166: x/evm/evmtest/tx.go#L166
Added line #L166 was not covered by tests
[warning] 170-171: x/evm/evmtest/tx.go#L170-L171
Added lines #L170 - L171 were not covered by tests
[warning] 174-174: x/evm/evmtest/tx.go#L174
Added line #L174 was not covered by tests
[warning] 184-184: x/evm/evmtest/tx.go#L184
Added line #L184 was not covered by tests
[warning] 220-222: x/evm/evmtest/tx.go#L220-L222
Added lines #L220 - L222 were not covered by tests
[warning] 225-225: x/evm/evmtest/tx.go#L225
Added line #L225 was not covered by tests
[warning] 237-248: x/evm/evmtest/tx.go#L237-L248
Added lines #L237 - L248 were not covered by tests
[warning] 250-255: x/evm/evmtest/tx.go#L250-L255
Added lines #L250 - L255 were not covered by tests
[warning] 257-261: x/evm/evmtest/tx.go#L257-L261
Added lines #L257 - L261 were not covered by tests
[warning] 263-264: x/evm/evmtest/tx.go#L263-L264
Added lines #L263 - L264 were not covered by tests
[warning] 280-280: x/evm/evmtest/tx.go#L280
Added line #L280 was not covered by tests
[warning] 283-283: x/evm/evmtest/tx.go#L283
Added line #L283 was not covered by tests
[warning] 295-295: x/evm/evmtest/tx.go#L295
Added line #L295 was not covered by tests
[warning] 299-301: x/evm/evmtest/tx.go#L299-L301
Added lines #L299 - L301 were not covered by testsx/evm/precompile/funtoken.go
[warning] 66-67: x/evm/precompile/funtoken.go#L66-L67
Added lines #L66 - L67 were not covered by tests
[warning] 69-70: x/evm/precompile/funtoken.go#L69-L70
Added lines #L69 - L70 were not covered by testsx/evm/precompile/precompile.go
[warning] 143-145: x/evm/precompile/precompile.go#L143-L145
Added lines #L143 - L145 were not covered by tests
[warning] 163-164: x/evm/precompile/precompile.go#L163-L164
Added lines #L163 - L164 were not covered by tests
[warning] 168-170: x/evm/precompile/precompile.go#L168-L170
Added lines #L168 - L170 were not covered by tests
[warning] 173-174: x/evm/precompile/precompile.go#L173-L174
Added lines #L173 - L174 were not covered by testsx/evm/precompile/wasm.go
[warning] 104-105: x/evm/precompile/wasm.go#L104-L105
Added lines #L104 - L105 were not covered by testsx/evm/statedb/journal.go
[warning] 161-172: x/evm/statedb/journal.go#L161-L172
Added lines #L161 - L172 were not covered by tests
[warning] 175-176: x/evm/statedb/journal.go#L175-L176
Added lines #L175 - L176 were not covered by testsx/evm/statedb/statedb.go
[warning] 512-519: x/evm/statedb/statedb.go#L512-L519
Added lines #L512 - L519 were not covered by tests
[warning] 525-532: x/evm/statedb/statedb.go#L525-L532
Added lines #L525 - L532 were not covered by tests
🪛 Gitleaks
x/evm/statedb/statedb.go
529-529: Identified a Vault Service Token, potentially compromising infrastructure security and access to sensitive credentials.
(vault-service-token)
🔇 Additional comments (29)
x/evm/statedb/interfaces.go (1)
17-17: Approve the change toAppendJournalEntrymethod signature.The change from
JournalEntrytoJournalChangealigns with the PR objectives and the broader restructuring of thestatedbpackage. This semantic shift in representing journal entries should improve clarity and maintainability.To ensure this change doesn't introduce any inconsistencies, please run the following script to verify that all implementations and usages of
ExtStateDBhave been updated accordingly:x/evm/evmtest/test_deps.go (1)
4-5: LGTM! Consider indirect testing for the new method.The addition of the
GoCtx()method is a valuable enhancement to theTestDepsstruct, providing a convenient way to obtain acontext.Contextfrom the existingsdk.Context. This change improves compatibility with Go's standard library and other packages that expect acontext.Context.While direct unit tests may not be necessary for this test utility file, it's important to ensure that the new method is indirectly tested through its usage in other test files. This will help maintain the reliability of the testing infrastructure.
To verify the usage and indirect testing of the new
GoCtx()method, you can run the following script:If the script doesn't return any results, consider adding usage of
GoCtx()in relevant test files to ensure it's working as expected.Would you like assistance in adding example usage or indirect tests for the
GoCtx()method?Also applies to: 59-62
✅ Verification successful
Verified! The
GoCtx()method is properly utilized in existing test files, ensuring adequate test coverage through indirect testing.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of the new GoCtx() method in test files # Test: Search for usage of GoCtx() method in test files rg -t go --glob '*_test.go' 'GoCtx\(\)'Length of output: 687
x/evm/logs_test.go (1)
196-199: LGTM! Consider verifying consistency across the codebase.The changes improve clarity by directly initializing the
evm.TransactionLogsstruct and ensure a consistent hexadecimal representation of the hash. This is a good refactor.To ensure consistency, please run the following script to check for any other occurrences of
NewTransactionLogsthat might need similar updates:x/evm/precompile/funtoken_test.go (4)
8-8: LGTM: Improved import alias for clarity.Renaming the Ethereum common package import to
gethcommonis a good practice. It helps avoid potential naming conflicts and makes the code more readable by clearly indicating the source of the imported package.
52-52: LGTM: Consistent updates to use renamed import.The changes in the TestFailToPackABI function correctly use the renamed
gethcommonpackage for address conversion. This maintains consistency with the import changes and doesn't alter the test logic.Also applies to: 58-58, 64-64
136-136: LGTM: Added VM error check.The addition of
s.Require().Empty(resp.VmError)is a valuable improvement. It ensures that the VM didn't encounter any errors during the contract call, adding an extra layer of validation to the test.
Line range hint
1-142: Overall assessment: Well-structured and improved test file.The changes in this file are consistent and improve the quality of the tests. The renaming of imports, updates to function calls, and modifications in assertions enhance clarity and robustness. The suggestions provided are minor and aimed at further improving readability and maintainability.
x/evm/keeper/funtoken_from_erc20_test.go (3)
163-163: LGTM: Improved function name clarityThe function name change from
TestSendFromEvmToCosmostoTestSendFromEvmToBankaccurately reflects the test's purpose and aligns with the PR objectives. This change improves code clarity and maintainability.
Line range hint
249-267: LGTM: Consistent refactoring and comprehensive balance checksThe changes in this section maintain consistency with the overall refactoring from "Cosmos" to "Bank". The test logic remains intact, ensuring continued coverage of the coin conversion functionality. The balance checks are comprehensive, covering both ERC20 and Bank balances, which is crucial for verifying the correctness of the conversion process.
Line range hint
1-290: Overall assessment: Improved test coverage and clarityThe changes in this file consistently refactor the terminology from "Cosmos" to "Bank", improving clarity and accurately representing the modules being tested. The addition of new error cases for both sending tokens to Bank and converting tokens back to ERC20 significantly enhances the test coverage, ensuring robust error handling in both directions.
The modifications align well with the PR objectives of addressing issues related to the EVM StateDB and improve the overall quality of the test suite. The suggested minor improvements in error checking and logging will further enhance the test robustness and debuggability.
Great job on maintaining and improving the test suite!
x/evm/statedb/state_object.go (2)
175-175: Improved clarity in balance change trackingThe renaming of
prevtoprevWeiin thebalanceChangestruct is a good improvement. It explicitly indicates that the previous balance is stored in Wei, which is consistent with theAccountWeirepresentation used in the EVM context. This change enhances code readability and reduces potential confusion about the unit of the stored balance.
Line range hint
1-292: Well-structured implementation of Ethereum account state managementThe overall implementation of Ethereum account state management in this file is well-structured and follows good practices. Key points:
- Clear separation between native (unibi) and Wei balance representations.
- Comprehensive methods for managing account attributes (balance, nonce, code, storage).
- Efficient use of caching and dirty state tracking.
- Proper journaling for state changes, allowing for reverting and snapshotting.
The code demonstrates a good understanding of EVM requirements and Cosmos SDK integration. Keep up the good work!
CHANGELOG.md (7)
Line range hint
15-136: Comprehensive update to the Unreleased sectionThe Unreleased section has been significantly expanded with a wide range of changes, including new features, improvements, bug fixes, and dependency updates. This level of detail is excellent for tracking the project's progress and upcoming changes.
Some notable additions include:
- Numerous improvements to the Ethereum Virtual Machine (EVM) integration
- Updates to various modules such as oracle, perp, and devgas
- Multiple dependency updates
Line range hint
138-141: Clear versioning and release informationThe changelog properly follows the Keep a Changelog format and adheres to Semantic Versioning. This is good practice and helps users understand the nature of changes between versions.
Line range hint
143-306: Detailed breakdown of changes for v1.0.0The changelog for v1.0.0 is well-organized and comprehensive, covering various aspects of the release including features, state machine breaking changes, and improvements. This level of detail is very helpful for users and developers.
Line range hint
308-374: Thorough documentation of changes for v0.21.11 and v0.21.10These sections provide a good overview of the changes in these versions, including links to specific pull requests. This level of detail helps in tracking the evolution of the project.
Line range hint
376-706: Comprehensive changelog for v0.19.4 and earlier versionsThe changelog continues to provide detailed information about changes, improvements, and bug fixes for earlier versions. This historical record is valuable for understanding the project's development over time.
Line range hint
708-1120: Consistent format and detail level maintained for older versionsThe changelog maintains a consistent format and level of detail even for older versions, which is commendable. It shows a commitment to good documentation practices throughout the project's history.
Line range hint
1-1120: Overall assessment of CHANGELOG.mdThis CHANGELOG.md file is exemplary in its structure, consistency, and level of detail. It effectively communicates the project's evolution over time, making it easier for users and developers to understand changes between versions.
Minor suggestions for improvement:
- Consider adding links to relevant issues or pull requests for each change, where applicable, to provide more context.
- For future entries, you might want to standardize the formatting of pull request numbers (e.g., always use the #XXX format).
x/evm/precompile/oracle.go (3)
27-27: Gas Calculation Now Utilizes Oracle ABIThe
RequiredGasfunction correctly calculates the gas requirement based on the input and the Oracle contract's ABI. This dynamic approach ensures that gas estimation is accurate for different function calls.
40-41: Improved Error Handling with Deferred FunctionThe use of
defer ErrPrecompileRun(err, p)()enhances the error handling mechanism by ensuring that any panic or error within theRunmethod is properly captured and reported. This aligns with best practices for robust error management.
45-45: Simplified Retrieval of Method, Args, and ContextAssigning
method, args, ctxfrom the result ofOnRunStartimproves code readability and maintains consistency with the new context management pattern introduced in the precompile package.x/evm/precompile/funtoken.go (2)
34-34: Improved gas calculation inRequiredGasfunctionThe
RequiredGasfunction now accurately calculates the gas cost using the contract's ABI, enhancing precision in gas estimation.
47-54: Refactored error handling and initialization inRunmethodThe introduction of
ErrPrecompileRun(err, p)()in thedeferstatement and the use ofOnRunStartstreamline error handling and improve code readability in theRunmethod.x/evm/precompile/precompile.go (1)
172-174: Handle error during context commit appropriatelyIn the
OnRunStartfunction, ifstateDB.CommitContext(cacheCtx)returns an error, you wrap it and return. Ensure that any necessary cleanup or state rollback is performed to maintain consistency, as failing to commit could leave the system in an unpredictable state.Please verify if additional steps are needed to safely handle this error scenario. If cleanup is required, consider implementing it to prevent potential issues.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 173-174: x/evm/precompile/precompile.go#L173-L174
Added lines #L173 - L174 were not covered by testsx/evm/statedb/journal.go (1)
269-276: Well-Documented Explanation of Access List ReversionThe additional comments clearly explain how
accessListAddAccountChangeandaccessListAddSlotChangeinteract during reversion. This enhances the readability and maintainability of the code.x/evm/evmtest/tx.go (2)
174-174: Check for VM Errors After Transaction ExecutionAfter executing the Ethereum transaction, it's good practice to check for VM errors and handle them appropriately.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 174-174: x/evm/evmtest/tx.go#L174
Added line #L174 was not covered by tests
225-225: Check for VM Errors After Transaction ExecutionAfter executing the transaction, check for any VM errors to ensure the transaction was successful.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 225-225: x/evm/evmtest/tx.go#L225
Added line #L225 was not covered by testsx/evm/precompile/wasm.go (1)
75-75: Adoption ofErrPrecompileRunEnhances Error HandlingThe introduction of
ErrPrecompileRunin theRunmethod improves error handling by providing structured error context without relying on reflection.
| return nil, err | ||
| } |
There was a problem hiding this comment.
Add unit tests for critical error handling paths
Static analysis indicates that lines 66-67 and 69-70 are not covered by tests. These error handling paths are crucial for ensuring robustness. Please consider adding unit tests to cover these scenarios and prevent potential regressions.
Would you like assistance in creating unit tests for these error conditions?
Also applies to: 69-70
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 66-67: x/evm/precompile/funtoken.go#L66-L67
Added lines #L66 - L67 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
CHANGELOG.md (5)
Line range hint
43-139: The "Unreleased" section provides a comprehensive overview of recent changes, but could benefit from some structural improvements.The organization of changes into "Nibiru EVM", "State Machine Breaking (Other)", and "Non-breaking/Compatible Improvements" is helpful for users to understand the impact of changes. However, consider the following suggestions:
- Add dates to the "Before Audit 2" and "Before Audit 1" subsections to provide better context for when these changes occurred.
- Ensure consistency in the formatting of pull request links. Some entries use full URLs while others use the shorter
#XXXXformat.- Consider grouping similar changes together within each section for easier readability.
Line range hint
139-185: The "State Machine Breaking (Other)" section could benefit from clearer organization and labeling.While it's good to separate state machine breaking changes, consider the following improvements:
- Clearly label each change as "BREAKING" at the beginning of the line for easy identification.
- Consider merging the "For next mainnet version" and "Dapp modules" subsections if there's significant overlap in the types of changes.
- Provide more context on the impact of these breaking changes, especially for major refactors like removing modules or changing data structures.
Line range hint
187-236: The "Non-breaking/Compatible Improvements" section could be better organized for readability.To improve this section:
- Consider categorizing changes into subsections such as "Features", "Refactoring", "Testing", and "Dependencies".
- Ensure consistent formatting for pull request references (e.g., always use the
#XXXXformat).- For dependency updates, consider grouping them together in a separate subsection or at the end of this section.
These changes would make it easier for users to quickly scan and understand the nature of the improvements.
Line range hint
238-284: The "Dependencies" section needs cleanup and a more concise format.To improve this section:
- Remove duplicate entries (e.g., entries for
github.com/CosmWasm/wasmvm,google.golang.org/grpc, etc.).- Consider using a table format or a more concise list for dependency updates, including the package name, version change, and PR number.
- Group similar updates together (e.g., all GitHub Actions related updates).
Example format:
- github.com/CosmWasm/wasmvm: 1.2.1 -> 1.4.0 (#1354, #1507, #1564) - google.golang.org/grpc: 1.53.0 -> 1.58.2 (#1395, #1437, #1443, #1497, #1525, #1568, #1582, #1598)This would make the section more readable and eliminate redundancy.
Line range hint
286-1037: The previous release entries are well-structured, but could benefit from some minor improvements for consistency.Positive aspects:
- Including links to releases and commits is excellent for traceability.
- Categorizing changes (e.g., "Features", "Bug Fixes", "Improvements") helps users quickly understand the nature of changes.
Suggestions for improvement:
- Maintain consistent section headers across all releases. For example, some releases use "API Breaking" while others use "Breaking Changes".
- Consider adding a brief summary of key changes at the beginning of each release entry, especially for major versions.
- Ensure all pull request references use the same format (preferably the
#XXXXformat for consistency and brevity).Overall, the changelog provides a comprehensive history of changes, which is crucial for users and developers. Implementing these minor improvements will enhance its readability and consistency.
x/evm/precompile/wasm.go (1)
103-105: Remove Redundant Error CheckThe error check at lines 103-105 is redundant since any errors should already be handled within each case of the switch statement.
You can remove these lines to simplify the code:
if err != nil { return nil, err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- CHANGELOG.md (3 hunks)
- x/evm/precompile/funtoken.go (3 hunks)
- x/evm/precompile/wasm.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/precompile/funtoken.go
🧰 Additional context used
🔇 Additional comments (1)
x/evm/precompile/wasm.go (1)
107-110:⚠️ Potential issueAdd Error Handling for
OnRunEndThe error returned by
OnRunEndat line 107 is not being checked. This could lead to unhandled errors affecting state consistency.Update the code to handle the error:
if err := OnRunEnd(res.StateDB, res.SnapshotBeforeRun, p.Address()); err != nil { return nil, err }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
x/evm/precompile/errors.go (1)
12-22: Approve the addition ofErrPrecompileRunwith minor suggestions.The new
ErrPrecompileRunfunction is a valuable addition that will standardize error handling for precompiled contracts. It effectively adds context to errors, which will aid in debugging. However, consider the following suggestions for improvement:
The use of reflection might have a slight performance impact. If the set of precompiled contracts is known and limited, consider using a type switch or a map of type names instead.
The function creates a new error rather than modifying the original. This approach might lead to loss of the original error's type information. Consider wrapping the original error instead of creating a new one.
Here's a potential refactoring to address these points:
func ErrPrecompileRun(err error, p vm.PrecompiledContract) func() { return func() { if err != nil { precompileType := getPrecompileTypeName(p) err = fmt.Errorf("precompile error: failed to run %s: %w", precompileType, err) } } } func getPrecompileTypeName(p vm.PrecompiledContract) string { switch p.(type) { case *PrecompileA: return "PrecompileA" case *PrecompileB: return "PrecompileB" // Add other precompile types here default: return reflect.TypeOf(p).Name() } }This refactoring suggestion maintains the use of reflection as a fallback, ensuring compatibility with any new precompile types that might be added in the future.
x/evm/precompile/precompile.go (3)
137-157: LGTM: Well-structured OnRunStartResult with clear documentation.The
OnRunStartResultstruct is a well-designed addition that encapsulates all necessary information for precompile execution. It provides a clean interface for managing state, context, and execution details.Consider adding a brief comment above the struct definition to explain its overall purpose and when it's used in the precompile execution flow.
159-212: LGTM: Well-implemented OnRunStart function with comprehensive setup.The
OnRunStartfunction effectively encapsulates the complex setup required for precompile execution. It handles input decoding, context creation, and state snapshot management in a clean and organized manner.Consider wrapping the error returned from
stateDB.CommitContextto provide more context:if err = stateDB.CommitContext(cacheCtx); err != nil { return res, fmt.Errorf("failed to commit dirty journal entries to cache context: %w", err) }This will make debugging easier by providing more specific information about where the error occurred.
234-244: LGTM: Clear categorization of precompile methods.The
precompileMethodIsTxMapprovides a concise and effective way to categorize precompile methods as transactions or queries. This is crucial for proper gas cost calculation and overall system behavior.Consider adding a brief comment above the map explaining its purpose and how it's used in the
RequiredGasfunction. This will improve maintainability and make it easier for future developers to understand the map's role in the system.x/evm/precompile/wasm.go (2)
Line range hint
36-67: Improved error handling and execution flow inRunmethod.The refactoring of the
Runmethod enhances error handling and introduces a more structured approach to executing precompiled contracts. The use ofOnRunStartandOnRunEndimproves the management of the execution lifecycle.Consider wrapping the
OnRunEndcall in a defer statement to ensure it's always called, even if an error occurs during the execution of the precompile logic:defer ErrPrecompileRun(err, p)() res, err := OnRunStart(evm, contract, p.ABI()) if err != nil { return nil, err } +defer func() { + if endErr := OnRunEnd(res.StateDB, res.SnapshotBeforeRun, p.Address()); endErr != nil { + err = endErr + } + res.WriteCtx() +}() method, args, ctx := res.Method, res.Args, res.Ctx // ... rest of the method ... -if err := OnRunEnd(res.StateDB, res.SnapshotBeforeRun, p.Address()); err != nil { - return nil, err -} -res.WriteCtx() returnThis ensures that
OnRunEndis always called and the context is always written back, even if an error occurs during the execution of the precompile logic.
79-81: AddedABImethod toprecompileWasmstruct.The new
ABImethod provides a centralized way to access the ABI for the Wasm precompile. This addition improves consistency and makes it easier to update the ABI in the future.Consider adding a brief comment to explain the purpose of this method:
+// ABI returns the Application Binary Interface (ABI) for the Wasm precompile contract. func (p precompileWasm) ABI() *gethabi.ABI { return embeds.SmartContract_Wasm.ABI }x/evm/statedb/journal.go (1)
96-132: LGTM: PrecompileSnapshotBeforeRun implementation looks good.The new
PrecompileSnapshotBeforeRunstruct and its methods are well-implemented and address the issue of reverting precompile changes that affect other modules. The comments provide a clear explanation of its purpose and functionality.Consider the following suggestions for potential improvements:
- Add error handling or logging in the
Revertmethod to capture any issues that might occur during the reversion process.- Consider adding a method to create a new
PrecompileSnapshotBeforeRuninstance, which could include any necessary validation or initialization logic.x/evm/statedb/statedb.go (3)
Line range hint
466-502: Improved context management and error handling in CommitContextThe renaming of
CommittoCommitContextand the addition of thectxparameter enhance flexibility in context management. The improved error handling is a positive change.However, there's a minor issue in the error formatting:
Please update the error messages to include the original error:
- return errorf("failed to delete account: %w") + return errorf("failed to delete account: %w", err)- return errorf("failed to set account: %w") + return errorf("failed to set account: %w", err)
496-502: Update function comments to match the 'Commit' methodThe addition of the
Commitmethod that callsCommitContextis good for maintaining backwards compatibility. However, the comments for this method should be updated to refer to[Commit]instead of[CommitContext].Here's the suggested change:
-// Commit writes the dirty journal state changes to the EVM Keeper. The -// StateDB object cannot be reused after [CommitContext] has completed. A new -// object needs to be created from the EVM. +// Commit writes the dirty journal state changes to the EVM Keeper. The +// StateDB object cannot be reused after [Commit] has completed. A new +// object needs to be created from the EVM.
513-523: Well-implemented caching mechanism for precompilesThe
CacheCtxForPrecompilemethod is a good addition for managing caching contexts in precompiled contracts. It properly initializes the cache context and provides necessary components for state management.Consider adding unit tests for this new method to ensure its correctness and prevent potential regressions in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- x/evm/precompile/errors.go (1 hunks)
- x/evm/precompile/oracle.go (1 hunks)
- x/evm/precompile/oracle_test.go (2 hunks)
- x/evm/precompile/precompile.go (2 hunks)
- x/evm/precompile/wasm.go (2 hunks)
- x/evm/statedb/journal.go (12 hunks)
- x/evm/statedb/statedb.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/precompile/oracle.go
🧰 Additional context used
🪛 Gitleaks
x/evm/statedb/statedb.go
540-540: Identified a Vault Service Token, potentially compromising infrastructure security and access to sensitive credentials.
(vault-service-token)
🔇 Additional comments (25)
x/evm/precompile/oracle_test.go (3)
30-30: LGTM: Consistent method name update.This change aligns with the previous update, ensuring consistency across different test cases within the
TestOracle_FailToPackABIfunction. The method name has been correctly updated toOracleMethod_queryExchangeRate.
67-67: LGTM: Consistent method name update in ABI unpacking.The change from
OracleMethod_QueryExchangeRatetoOracleMethod_queryExchangeRatein theTestOracle_HappyPathfunction maintains consistency with the previous updates. This ensures that the method name used for unpacking the ABI response aligns with the updated method name used elsewhere in the codebase.
Line range hint
24-67: Summary: Consistent method name updates across test cases.The changes in this file consistently update the method name from
OracleMethod_QueryExchangeRatetoOracleMethod_queryExchangeRateacross different test functions and use cases. These updates improve alignment between the test file and the implementation file, enhancing overall code consistency without altering the functional behavior of the tests.x/evm/precompile/errors.go (1)
12-22: Document usage and ensure consistent application ofErrPrecompileRun.The introduction of
ErrPrecompileRunis a positive step towards standardizing error handling for precompiled contracts. To maximize its benefits:
Consider adding a comment explaining the intended usage pattern, e.g.:
// ErrPrecompileRun is intended to be used in a defer pattern: // defer ErrPrecompileRun(err, p)()Ensure that this function is consistently used across all precompiled contract implementations in the codebase.
To verify consistent usage, you can run the following script:
This script will help identify any precompiled contracts that might not be using the new error handling mechanism.
x/evm/precompile/precompile.go (4)
27-30: LGTM: Import additions are appropriate.The new imports for
sdkandstatedbare necessary and correctly align with the new functionality introduced in this file.
214-232: LGTM: Well-implemented OnRunEnd function for state consistency.The
OnRunEndfunction is a crucial addition that ensures proper state management after precompile execution. It effectively handles saving the state snapshot to the journal, which is essential for maintaining consistency across various scenarios, including reverts and multi-precompile transactions.The implementation and documentation are clear and comprehensive.
Line range hint
1-244: Overall assessment: Significant improvements in precompile execution managementThe changes introduced in this file represent a substantial enhancement to the precompile package. The new
OnRunStartResultstruct, along with theOnRunStartandOnRunEndfunctions, provide a robust framework for managing precompile execution, state isolation, and consistency. The code is well-structured, thoroughly documented, and introduces important safeguards against potential state inconsistencies.Key improvements:
- Better encapsulation of precompile execution setup and teardown.
- Improved state management and isolation.
- Clear categorization of precompile methods for gas cost calculation.
While the implementation is solid, consider addressing the following to further enhance the code:
- Add brief comments explaining the overall purpose of new structures and maps.
- Improve error wrapping in some areas for better debugging.
- Ensure comprehensive test coverage, especially for critical error paths.
These changes significantly improve the robustness and maintainability of the precompile package.
159-212: Increase test coverage for critical error pathsWhile the implementation of
OnRunStartlooks solid, it's important to ensure that all critical error paths are covered by tests. This includes scenarios such as:
- Failure to decode input
- Failure to load the SDK context from the EVM StateDB
- Failure to commit dirty journal entries
Adding comprehensive unit tests for these scenarios will improve the robustness and reliability of the precompile execution flow.
To help verify the current test coverage, you can run the following command:
This will show the current test coverage for the
OnRunStartfunction. If the coverage is below 80%, consider adding more test cases to cover the critical error paths.x/evm/precompile/wasm.go (3)
71-73: SimplifiedprecompileWasmstruct definition.The
precompileWasmstruct has been streamlined to contain only aWasmfield. This simplification suggests improved encapsulation of functionality within theWasmtype.
75-77: AddedAddressmethod toprecompileWasmstruct.The new
Addressmethod provides a clear way to obtain the precompile's address. This addition improves code readability and maintainability by centralizing the address definition.
83-86: UpdatedRequiredGasmethod to useABImethod.The
RequiredGasmethod now usesp.ABI()instead of directly accessingembeds.SmartContract_Wasm.ABI. This change improves consistency and maintainability by centralizing the ABI access.x/evm/statedb/journal.go (11)
24-25: LGTM: New imports added correctly.The new imports for
storeandsdkfrom the Cosmos SDK are correctly added and aliased. These are likely necessary for the newPrecompileSnapshotBeforeRunfunctionality.
Line range hint
29-37: LGTM: JournalEntry renamed to JournalChange.The renaming of
JournalEntrytoJournalChangeis consistent and provides a clearer description of the interface's purpose. The updated comment accurately reflects this change while maintaining clarity about the interface's functionality.
43-43: LGTM: Journal entries type updated.The
entriesfield in thejournalstruct has been correctly updated to use[]JournalChange, maintaining consistency with the interface renaming.
67-67: LGTM: Append method parameter type updated.The
appendmethod's parameter type has been correctly updated fromJournalEntrytoJournalChange, maintaining consistency with the interface renaming.
Line range hint
134-152: LGTM: createObjectChange implementation is correct.The
createObjectChangestruct and its methods are well-implemented. TheRevertmethod correctly deletes the state object for the account, and theDirtiedmethod returns the appropriate address. The comment clearly explains the purpose of this struct.
Line range hint
153-172: LGTM: resetObjectChange implementation is correct.The
resetObjectChangestruct and its methods are well-implemented. TheRevertmethod correctly sets the state object to its previous version, and theDirtiedmethod appropriately returns nil. The comment clearly explains the purpose of this struct.
Line range hint
173-214: LGTM: suicideChange and balanceChange updates improve clarity and functionality.The updates to
suicideChangeandbalanceChangestructs are well-implemented:
- The addition of
prevbalancetosuicideChangeallows for more accurate state reversion.- Renaming
prevtoprevWeiinbalanceChangeimproves clarity.These changes are consistent with the overall refactoring and enhance the functionality of the journal system.
Line range hint
215-275: LGTM: nonceChange, codeChange, and storageChange implementations are correct and consistent.The new
nonceChange,codeChange, andstorageChangestructs are well-implemented:
- Each struct correctly implements the
JournalChangeinterface.- The
Revertmethods appropriately restore the previous state.- The
Dirtiedmethods return the correct account address.The comments clearly explain the purpose of each struct, maintaining consistency with the overall design of the journal system.
Line range hint
276-311: LGTM: refundChange and addLogChange implementations are correct and consistent.The new
refundChangeandaddLogChangestructs are well-implemented:
- Both correctly implement the
JournalChangeinterface.- The
Revertmethods appropriately handle their respective state changes.- The
Dirtiedmethods correctly return nil as these changes don't affect specific accounts.The comments clearly explain the purpose of each struct, maintaining consistency with the overall design of the journal system.
Line range hint
312-357: LGTM: accessListAddAccountChange and accessListAddSlotChange implementations are correct and well-documented.The new
accessListAddAccountChangeandaccessListAddSlotChangestructs are well-implemented:
- Both correctly implement the
JournalChangeinterface.- The
Revertmethods appropriately handle their respective access list changes.- The
Dirtiedmethods correctly return nil as these changes don't affect account state directly.The comments provide clear explanations of each struct's purpose and the relationship between them, which is particularly helpful for understanding the two-step process of adding an (address, slot) combination to the access list.
Line range hint
1-357: Overall: Excellent refactoring and extension of the journal system.The changes in this file significantly improve the journal system:
- Renaming
JournalEntrytoJournalChangeprovides better clarity on the interface's purpose.- New structs for various state changes (e.g.,
PrecompileSnapshotBeforeRun,createObjectChange,resetObjectChange, etc.) enhance the system's ability to handle different types of state modifications.- Consistent implementation of the
JournalChangeinterface across all new structs ensures a uniform approach to state reversion.- Clear and detailed comments throughout the file explain the purpose and functionality of each new component.
These changes should result in a more robust and flexible state management system for the EVM implementation.
x/evm/statedb/statedb.go (3)
33-47: Excellent additions to enhance StateDB functionalityThe new fields
cacheCtx,writeCacheCtxFn, andprecompileSnapshotsCountalong with their clear explanations improve theStateDBstruct's capability to handle precompiled contracts and caching. These changes appear to be well-thought-out and properly documented.
Line range hint
1-546: Overall excellent improvements to StateDBThe changes in this file significantly enhance the functionality of
StateDBfor handling precompiled contracts and improving state management. The new methods and fields are well-documented and align with the PR objectives.Key improvements:
- Enhanced context management with
CacheCtxForPrecompile- Improved state management for precompiled contracts with
SavePrecompileSnapshotToJournal- Better error handling in the
CommitContextmethodSuggestions for further improvement:
- Add unit tests for the new methods
CacheCtxForPrecompileandSavePrecompileSnapshotToJournal- Consider making
maxPrecompileCallsconfigurable- Update comments for the
Commitmethod to differentiate it fromCommitContextOverall, these changes represent a significant improvement to the codebase.
🧰 Tools
🪛 Gitleaks
540-540: Identified a Vault Service Token, potentially compromising infrastructure security and access to sensitive credentials.
(vault-service-token)
525-544: Robust implementation of precompile snapshot managementThe
SavePrecompileSnapshotToJournalmethod is a well-implemented addition that enhances state management for precompiled contracts. The limit check onprecompileSnapshotsCountis a good safeguard against potential abuse.Consider adding unit tests for this new method to ensure its correctness and prevent potential regressions.
The static analysis tool flagged a potential Vault Service Token on line 540. While this appears to be a false positive (as the line contains a comparison against
maxPrecompileCalls), it's worth double-checking to ensure no sensitive information is inadvertently included. Please run the following command to verify:🧰 Tools
🪛 Gitleaks
540-540: Identified a Vault Service Token, potentially compromising infrastructure security and access to sensitive credentials.
(vault-service-token)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
x/evm/precompile/oracle.go (1)
Inconsistent error message for expected arguments
The error message in
x/evm/precompile/oracle.gostill expects 3 arguments instead of 1. Please update all related error messages to ensure consistency.
x/evm/precompile/oracle.go: expected 3 arguments but got %d🔗 Analysis chain
Line range hint
93-93: Corrected error message in decomposeQueryExchangeRateArgsThe error message has been updated to correctly state the expected number of arguments (1 instead of 3). This improves the accuracy of error reporting and will help developers debug issues more effectively.
To ensure consistency, please run the following command to check for any similar error messages that might need updating:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar error messages about expected arguments rg "expected \d+ arguments but got"Length of output: 129
x/evm/precompile/wasm.go (2)
73-75: Approve simplifiedprecompileWasmstruct with a suggestion for documentation.The simplification of the
precompileWasmstruct improves code clarity. However, adding documentation would enhance understanding of its purpose and usage.Consider adding a comment to explain the purpose of the
precompileWasmstruct and itsWasmfield. For example:// precompileWasm represents a precompiled contract for WebAssembly (Wasm) execution in the EVM. type precompileWasm struct { // Wasm contains the necessary components for Wasm contract operations. Wasm Wasm }
97-104: Approve update toPrecompileWasmfunction with a suggestion for return type.The modification to return a
precompileWasmstruct aligns with the updated struct definition and ensures proper implementation of thevm.PrecompiledContractinterface.Consider updating the function signature to explicitly return
precompileWasmfor improved clarity:func PrecompileWasm(keepers keepers.PublicKeepers) precompileWasm { // ... (existing implementation) }This change makes it clear that the function returns a specific implementation of
vm.PrecompiledContract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- x/evm/precompile/errors.go (1 hunks)
- x/evm/precompile/funtoken.go (3 hunks)
- x/evm/precompile/oracle.go (1 hunks)
- x/evm/precompile/wasm.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/evm/precompile/errors.go
- x/evm/precompile/funtoken.go
🧰 Additional context used
🔇 Additional comments (6)
x/evm/precompile/oracle.go (3)
27-27: Improved gas calculation for precompiled contractThe update to use
RequiredGas(input, embeds.SmartContract_Oracle.ABI)is a good improvement. This change allows for more accurate and dynamic gas calculation based on the contract's ABI, which is a best practice for precompiled contracts.
39-51: Improved error handling and method execution in RunThe changes in the
Runmethod are well-implemented:
- The new error handling using
ErrPrecompileRunsimplifies the code and improves consistency.- The introduction of
OnRunStartcentralizes common operations, enhancing maintainability.- The switch case update aligns with the constant name change, maintaining consistency.
These improvements enhance the overall structure and readability of the code.
31-31: Naming convention update for Oracle method constantThe renaming of
OracleMethod_QueryExchangeRatetoOracleMethod_queryExchangeRateimproves consistency with Go naming conventions for constants. This is a good change.To ensure consistency across the codebase, please run the following command to check for any remaining occurrences of the old name:
x/evm/precompile/wasm.go (3)
77-83: Approve newAddressandABImethods.The addition of
Address()andABI()methods enhances the functionality of theprecompileWasmstruct, aligning it with thevm.PrecompiledContractinterface requirements.As mentioned in a previous review comment, please add documentation to the
ABI()method to improve code readability and maintainability.
85-88: Approve update toRequiredGasmethod.The modification to use
p.ABI()instead of directly accessingembeds.SmartContract_Wasm.ABIimproves consistency and maintainability by centralizing ABI access through theABI()method.
Line range hint
37-69: Approve changes to theRunmethod with a suggestion for verification.The refactoring of the
Runmethod improves error handling and introduces a more structured approach to running precompiles. The use ofOnRunStartandOnRunEndfunctions enhances execution management and state consistency.Please verify that all callers of this method have been updated to handle the removal of the
bz []bytereturn value. Run the following script to find potential callers:✅ Verification successful
Verified: No external callers of
precompileWasm.Runfound.The
Runmethod inprecompileWasmis only invoked within test files. There are no production code dependencies affected by the removal of thebz []bytereturn value.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find potential callers of the Run method rg -i 'precompileWasm.*Run\s*\(' -g '!x/evm/precompile/wasm.go'Length of output: 65
Script:
#!/bin/bash # Broader search for Run method invocations rg -i 'Run\s*\(' # Use ast-grep to find method calls to Run on precompileWasm ast-grep 'precompileWasm.Run($_)' --lang goLength of output: 17267
Script:
#!/bin/bash # Correct usage of ast-grep to find method calls to Run on precompileWasm ast-grep 'precompileWasm.Run($_)' --lang go # Alternatively, use rg with a more precise pattern to match method receivers rg -i 'precompileWasm\.Run\s*\('Length of output: 200
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (12)
x/evm/evmtest/erc20.go (1)
103-107: Approve the addition ofBigPowfunction with a minor suggestion.The
BigPowfunction is a useful utility for EVM-related calculations, especially when dealing with token amounts and decimals. Its implementation is correct and efficient.Consider adding a simple example in the function's comment to illustrate its usage:
// BigPow multiplies "amount" by 10 to the "pow10Exp". // For example, BigPow(big.NewInt(5), 18) returns 5 * 10^18. func BigPow(amount *big.Int, pow10Exp uint8) (powAmount *big.Int) { // ... (existing implementation) }This addition would enhance the function's documentation and make it more developer-friendly.
x/evm/errors.go (1)
77-87: Improved error handling, with minor suggestions for further enhancement.The changes to
NewExecErrorWithReasonfunction significantly improve error handling and provide more consistent and informative error messages. The new approach handles different scenarios effectively, including cases where unpacking fails but a result is still available.Consider the following minor improvements:
- Move the
reasonvariable declaration inside the first if block since it's only used there:errPrefix := "execution reverted" if errUnpack == nil { + reason, _ := abi.UnpackRevert(result) reasonStr := reason err = fmt.Errorf("%s with reason \"%v\"", errPrefix, reasonStr) } else if string(result) != "" {
- Differentiate error messages for unpacked and raw results:
if errUnpack == nil { reason, _ := abi.UnpackRevert(result) - err = fmt.Errorf("%s with reason \"%v\"", errPrefix, reasonStr) + err = fmt.Errorf("%s with unpacked reason \"%v\"", errPrefix, reason) } else if string(result) != "" { reasonStr := string(result) - err = fmt.Errorf("%s with reason \"%v\"", errPrefix, reasonStr) + err = fmt.Errorf("%s with raw reason \"%v\"", errPrefix, reasonStr) }
- Consider using error wrapping for better error handling up the call stack:
if errUnpack == nil { reason, _ := abi.UnpackRevert(result) - err = fmt.Errorf("%s with unpacked reason \"%v\"", errPrefix, reason) + err = fmt.Errorf("%s with unpacked reason: %w", errPrefix, errors.New(reason)) } else if string(result) != "" { reasonStr := string(result) - err = fmt.Errorf("%s with raw reason \"%v\"", errPrefix, reasonStr) + err = fmt.Errorf("%s with raw reason: %w", errPrefix, errors.New(reasonStr)) } else { - err = errors.New(errPrefix) + err = fmt.Errorf("%s: %w", errPrefix, errors.New("no additional information")) }These changes would further improve the clarity and utility of the error messages while maintaining the robust error handling you've implemented.
x/evm/evmtest/tx.go (5)
128-130: LGTM: Improved transaction generation and error handlingThe changes enhance the transaction generation process by separating it from signing and adding immediate error checking. This improves the overall robustness of the function.
Consider consolidating the error checks for both generation and signing:
ethTxMsg, gethSigner, krSigner, err := GenerateEthTxMsgAndSigner(txArgs, deps, deps.Sender) -require.NoError(t, err) -err = ethTxMsg.Sign(gethSigner, krSigner) -require.NoError(t, err) +require.NoError(t, err) +require.NoError(t, ethTxMsg.Sign(gethSigner, krSigner))This minor refactor would make the code slightly more concise while maintaining the same functionality.
160-170: Simplify error handling in DeployContractThe changes improve the transaction generation and signing process. However, the error handling structure can be simplified for better readability and maintainability.
Consider refactoring the error handling as follows:
ethTxMsg, gethSigner, krSigner, err := GenerateEthTxMsgAndSigner( evm.JsonTxArgs{ Nonce: (*hexutil.Uint64)(&nonce), Input: (*hexutil.Bytes)(&bytecodeForCall), From: &deps.Sender.EthAddr, }, deps, deps.Sender, ) if err != nil { return nil, errors.Wrap(err, "failed to generate eth tx msg") -} else if err := ethTxMsg.Sign(gethSigner, krSigner); err != nil { +} +if err := ethTxMsg.Sign(gethSigner, krSigner); err != nil { return nil, errors.Wrap(err, "failed to sign eth tx msg") }This change eliminates the
else ifclause, making the error handling more straightforward and easier to read.
193-197: LGTM: Enhanced DeployAndExecuteERC20Transfer functionThe changes improve the function by providing more return values and using the new
GenerateEthTxMsgAndSignermethod. The contract address calculation move enhances readability.Consider consolidating the error checks for both generation and signing, similar to the suggestion for
ExecuteNibiTransfer:erc20Transfer, gethSigner, krSigner, err := GenerateEthTxMsgAndSigner(txArgs, deps, deps.Sender) -require.NoError(t, err) -err = erc20Transfer.Sign(gethSigner, krSigner) -require.NoError(t, err) +require.NoError(t, err) +require.NoError(t, erc20Transfer.Sign(gethSigner, krSigner))This minor refactor would make the code slightly more concise while maintaining the same functionality.
Also applies to: 205-205, 223-225
235-261: LGTM: New CallContractTx functionThe new
CallContractTxfunction provides a clean abstraction for calling a contract, which will improve code reusability and readability in tests. It correctly uses the newGenerateEthTxMsgAndSignermethod and handles errors appropriately.Consider using
errors.Wrapfor consistency with error handling in other functions:if err != nil { - err = fmt.Errorf("CallContract error during tx generation: %w", err) + return nil, nil, errors.Wrap(err, "CallContract error during tx generation") return } err = ethTxMsg.Sign(gethSigner, krSigner) if err != nil { - err = fmt.Errorf("CallContract error during signature: %w", err) + return nil, nil, errors.Wrap(err, "CallContract error during signature") return }This change would make the error handling consistent with other parts of the codebase and provide stack traces for easier debugging.
Line range hint
263-297: LGTM: New GenerateEthTxMsgAndSigner functionThe new
GenerateEthTxMsgAndSignerfunction centralizes the logic for generating Ethereum transactions and signers, which will improve consistency across the codebase. The inclusion of a usage example in the documentation is excellent and enhances code understandability.Consider using named return values for clarity and to ensure all return values are set in error cases:
func GenerateEthTxMsgAndSigner( jsonTxArgs evm.JsonTxArgs, deps *TestDeps, sender EthPrivKeyAcc, -) (evmTxMsg *evm.MsgEthereumTx, gethSigner gethcore.Signer, krSigner keyring.Signer, err error) { +) (evmTxMsg *evm.MsgEthereumTx, gethSigner gethcore.Signer, krSigner keyring.Signer, err error) { estimateArgs, err := json.Marshal(&jsonTxArgs) if err != nil { - return + return nil, nil, nil, err } // ... (rest of the function) if err != nil { - return + return nil, nil, nil, err } // ... (rest of the function) }This change ensures that all return values are explicitly set in error cases, improving code clarity and preventing potential issues with uninitialized return values.
x/evm/keeper/grpc_query_test.go (2)
794-794: Consider capturing the third return valueThe
evmtest.DeployAndExecuteERC20Transferfunction now returns three values, but the third value is being discarded. Consider capturing this value and using it if it contains relevant information for the test.-txMsg, predecessors, _ := evmtest.DeployAndExecuteERC20Transfer(deps, s.T()) +txMsg, predecessors, thirdValue := evmtest.DeployAndExecuteERC20Transfer(deps, s.T())
873-873: Consider capturing discarded return valuesThe
evmtest.DeployAndExecuteERC20Transferfunction now returns three values, but the second and third values are being discarded. Consider capturing these values and using them if they contain relevant information for the test.-txMsg, _, _ := evmtest.DeployAndExecuteERC20Transfer(deps, s.T()) +txMsg, secondValue, thirdValue := evmtest.DeployAndExecuteERC20Transfer(deps, s.T())x/evm/evmtest/tx_test.go (3)
1-1: Correct the company name in the copyrightThe company name appears to be misspelled. It should be "Nibiru Labs, Inc." instead of "Nibi, Inc."
Apply this diff to correct the company name:
-// Copyright (c) 2023-2024 Nibi, Inc. +// Copyright (c) 2023-2024 Nibiru Labs, Inc.
26-26: Provide a descriptive message for error handlingWhen checking for errors with
s.Require().NoError, consider providing a descriptive message to aid in debugging if the test fails.Apply this diff to include an error message:
-s.Require().NoError(err, deployResp) +s.Require().NoError(err, "Failed to deploy contract: %v", err)
78-86: Assert the recipient's balance after the transferTo fully verify the transfer, add an assertion to check that the recipient's bank balance has increased by the transferred amount.
Apply this diff to assert the recipient's balance:
evmtest.AssertBankBalanceEqual( s.T(), deps, evm.EVMBankDenom, deps.Sender.EthAddr, big.NewInt(69_000), ) + +// Assert recipient's bank balance +evmtest.AssertBankBalanceEqual( + s.T(), deps, evm.EVMBankDenom, to, big.NewInt(420), +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- x/evm/errors.go (1 hunks)
- x/evm/evmtest/erc20.go (2 hunks)
- x/evm/evmtest/tx.go (7 hunks)
- x/evm/evmtest/tx_test.go (1 hunks)
- x/evm/keeper/grpc_query.go (1 hunks)
- x/evm/keeper/grpc_query_test.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
x/evm/evmtest/erc20.go (1)
26-26: Consider the implications of string comparison for balance assertion.The change from numerical comparison to string comparison for ERC20 balances might have both advantages and drawbacks:
Pros:
- Improved readability in test failure messages.
- Avoids potential issues with
big.Intcomparison semantics.Cons:
- Potential loss of precision for very large numbers due to string representation.
- Slight performance overhead due to string conversion.
To ensure this change doesn't introduce any regressions, please run the following script to check for any existing tests that might be affected:
Consider adding a comment explaining the rationale behind this change to help future maintainers understand the decision.
x/evm/evmtest/tx.go (2)
23-24: LGTM: Keyring import addedThe addition of the
keyringimport is consistent with the changes in transaction handling and signing observed throughout the file.
Line range hint
1-345: Overall assessment: Significant improvements in transaction handlingThe changes in this file represent a substantial improvement in the handling of Ethereum transactions within the test environment. Key enhancements include:
- Centralization of transaction generation and signing logic with the new
GenerateEthTxMsgAndSignerfunction.- Improved error handling and reporting across various functions.
- Addition of the
CallContractTxfunction for a cleaner abstraction of contract interactions.- Consistent use of the new transaction generation method throughout the file.
These changes align well with the PR objectives of addressing issues related to the Ethereum Virtual Machine (EVM) StateDB. The modifications should lead to more robust and maintainable test code, potentially reducing the likelihood of bugs stemming from inconsistent transaction handling.
While the changes are generally excellent, consider implementing the minor suggestions provided in the individual comments to further enhance code clarity and consistency.
x/evm/keeper/grpc_query.go (2)
666-673: LGTM! Improved context setup.The refactoring of the context setup into a single chained method call enhances readability and conciseness. The functionality remains unchanged while the code becomes more maintainable.
666-673: Summary: Improved context setup in TraceBlock methodThe changes in this file are focused on refactoring the context setup in the
TraceBlockmethod. This refactoring improves code readability and maintainability without altering the functionality. The rest of the file, including other methods and theTraceEthTxMsgfunction, remains unchanged and continues to work correctly with the modified context.
…tps://github.com/NibiruChain/nibiru into ud/evm-fix-state-not-committed-before-precompile
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (16)
x/evm/statedb/journal_test.go (3)
24-24: Consider defining the funding amount as a named constant.The magic number
69_420should be defined as a constant with a meaningful name that reflects its purpose in the test.+const testFundingAmount = 69_420 - sdk.NewCoins(sdk.NewCoin(bankDenom, sdk.NewInt(69_420))), + sdk.NewCoins(sdk.NewCoin(bankDenom, sdk.NewInt(testFundingAmount))),
34-66: Consider documenting the counter value progression.The test uses specific counter values (0 -> 7 -> 12) but doesn't document why these specific values were chosen. Consider adding a comment explaining the test scenario or using named constants.
+ // Test increments: 0 (initial) -> +7 -> +5 = 12 (final) stateTransitions := []Transition{
81-81: Document the expected journal entries count.The test expects exactly 13 journal entries but doesn't explain why this number is expected. Consider adding a comment explaining the breakdown of journal entries or define it as a named constant.
+ // 13 entries: 1 for initial state + 7 for first increment + 5 for second increment s.Require().Len(entries, 13, "expect 13 journal entries")x/evm/keeper/funtoken_from_coin.go (1)
Line range hint
81-87: Consider explicit error wrapping with contextWhile the error handling includes wrapping, consider adding more context about the deployment state:
- return gethcommon.Address{}, errors.Wrap(err, "failed to deploy ERC20 contract") + return gethcommon.Address{}, errors.Wrapf(err, "failed to deploy ERC20 contract for bank coin %s", bankCoin.Name)x/evm/precompile/wasm_test.go (1)
Line range hint
309-313: Consider enhancing error message specificity.While the error handling is correct, consider making the error messages more specific to the EVM context for better debugging. For example, include whether the error occurred during precompile execution or state transition.
CHANGELOG.md (3)
58-59: Verify the precompile state fix descriptionThe description of PR #2086 could be more detailed to explain:
- The root cause of the stale state issue
- The specific impact on precompile execution
- How committing state at precompile start fixes the issue
Consider expanding the description to:
- fix(evm-precompiles): It is possible for the `Run` function of a custom precompile to retrieve stale state because EVM state changes can occur before the precompile is called that are recorded as entries in the StateDB journal for the transaction without being reflected in the `sdk.Context`. This pull request makes sure that state is committed as expected. + fix(evm-precompiles): Fixed a critical issue where custom precompile's `Run` function could retrieve stale state. This occurred because EVM state changes before the precompile call were recorded in the StateDB journal but not reflected in the `sdk.Context`. The fix ensures state consistency by committing temporary state at precompile start, preventing any uncommitted journal entries from causing incorrect state reads. This resolves potential issues with precompile execution operating on outdated state.
43-51: Improve changelog section organizationThe "Before Audit 2" section would benefit from better organization:
- Group related changes together (e.g., all ERC20/FunToken changes)
- Add subsection headers for different types of changes
- Include the audit timeline/scope information in a more structured way
Consider restructuring as:
#### Nibiru EVM | Before Audit 2 [Nov, 2024] ##### Audit Context The codebase underwent a Code4rena Zenith Audit from 2024-10-07 to 2024-11-01, including both primary review and mitigation periods. The following changes were made after that audit in preparation for a second audit in November 2024. ##### Critical Fixes - [#2086](https://github.com/NibiruChain/nibiru/pull/2086) - fix(evm-precompiles): Fixed state consistency issues in precompile execution... ##### ERC20 Improvements - [#2074](https://github.com/NibiruChain/nibiru/pull/2074) - fix(evm-keeper): Improved ERC20 metadata handling during FunToken creation... ##### Gas Optimizations - [#2076](https://github.com/NibiruChain/nibiru/pull/2076) - fix(evm-gas-fees): Optimized gas price calculations...
Line range hint
1-42: Enhance changelog header documentationThe changelog header comments could be improved to better guide contributors on changelog maintenance.
Add the following sections to the header:
<!-- Additional Guidelines: 1. Version Sections: - Use ### for major versions (v1.0.0) - Use #### for minor versions (v1.1.0) - Use ##### for patch versions (v1.1.1) 2. Change Categories: - Group changes into consistent categories: - 🚨 Critical - Breaking changes requiring immediate attention - 🔒 Security - Security related changes - 🔄 State Machine Breaking - 🛠 Features - 🐛 Bug Fixes - ⚡️ Performance - 📚 Documentation 3. Pull Request References: - Always include PR links in format [#1234](url) - Add context about why changes were made -->x/evm/precompile/precompile.go (1)
154-154: Correct typo in documentation comment.There's a minor typo in the comment for
OnRunStart:
- "It handles decoding the input data according the to contract ABI..." should be "It handles decoding the input data according to the contract ABI..."
x/evm/keeper/erc20.go (3)
61-62: Consider handling the unusedevmObjreturn value fromCallContractWithInputThe function
CallContractWithInputnow returns an additionalevmObj *vm.EVM, which is currently being ignored in:
Mintfunction (lines 61-62)Transferfunction (line 83)Burnfunction (lines 123-124)CallContractfunction (lines 155-156)If the
evmObjis unnecessary for the current implementation, consider explicitly acknowledging the unused return value to improve code clarity. This can be done by assigning it to the blank identifier_.Apply this diff to acknowledge the unused return value:
- evmResp, _, err = e.CallContractWithInput(ctx, from, &contract, true, input) + evmResp, _, err = e.CallContractWithInput(ctx, from, &contract, true, input) // evmObj is intentionally ignored // Similarly for the other functions: - resp, _, err := e.CallContractWithInput(ctx, from, &contract, true, input) + resp, _, err := e.CallContractWithInput(ctx, from, &contract, true, input) // evmObj is intentionally ignored - evmResp, _, err = e.CallContractWithInput(ctx, from, &contract, commit, input) + evmResp, _, err = e.CallContractWithInput(ctx, from, &contract, commit, input) // evmObj is intentionally ignored - evmResp, _, err = k.CallContractWithInput(ctx, fromAcc, contract, commit, contractInput) + evmResp, _, err = k.CallContractWithInput(ctx, fromAcc, contract, commit, contractInput) // evmObj is intentionally ignoredAlso applies to: 83-83, 123-124, 155-156
217-218: Useerrors.Wrapinstead oferrors.Wrapfwhen no formatting is neededAt lines 217-218,
errors.Wrapfis used without any formatting directives. Replaceerrors.Wrapfwitherrors.Wrapfor clearer intent.Apply this diff:
- err = errors.Wrapf(err, "failed to load evm config") + err = errors.Wrap(err, "failed to load evm config")
229-230: Useerrors.Wrapinstead oferrors.Wrapfwhen no formatting is neededAt lines 229-230,
errors.Wrapfis used without any formatting directives. Useerrors.Wrapinstead to accurately reflect that no formatting is required.Apply this diff:
- err = errors.Wrapf(err, "failed to apply EVM message") + err = errors.Wrap(err, "failed to apply EVM message")x/evm/keeper/msg_server.go (3)
Line range hint
292-323: Consistent return values in error casesIn several error return statements,
evmObjis returned even when errors occur. This might be acceptable ifevmObjprovides useful context even during errors. However, ifevmObjis not fully initialized or may benil, it's safer to returnnilforevmObjin error cases to avoid unintended consequences.Consider returning
nilforevmObjin error cases to prevent potential nil pointer dereferences.Suggested changes:
core.ErrIntrinsicGas, "apply message msg.Gas = %d, intrinsic gas = %d.", leftoverGas, intrinsicGas, )
372-372: Handle errors duringstateDBcommitIn the
ApplyEvmMsgfunction, if an error occurs duringstateDB.Commit(), the function returns the error. Ensure that any necessary cleanup or rollback is performed in this case to maintain state consistency.Consider adding logging or additional error handling to capture the context of the commit failure.
if err := stateDB.Commit(); err != nil { k.Logger(ctx).Error("stateDB commit failed", "error", err) return nil, evmObj, fmt.Errorf("failed to commit stateDB: %w", err) }
381-385: Improve error messages for gas calculationsThe error messages in returns at lines 381 and 385 could be more informative by including the relevant variable values.
Ensure that the error messages clearly convey the cause of the error.
Example:
x/evm/precompile/test/export.go (1)
58-64: Consistent Usage of s.Require().NoError for Critical ErrorsInconsistent use of
s.NoError(err)ands.Require().NoError(err)can lead to tests continuing after critical errors. For example, at line 58 you uses.NoError(err), but subsequent code depends onerrbeing nil. Uses.Require().NoError(err)consistently to ensure the test halts upon encountering an error that impacts subsequent assertions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- CHANGELOG.md (3 hunks)
- x/evm/keeper/erc20.go (8 hunks)
- x/evm/keeper/funtoken_from_coin.go (1 hunks)
- x/evm/keeper/grpc_query.go (5 hunks)
- x/evm/keeper/msg_server.go (10 hunks)
- x/evm/precompile/funtoken.go (3 hunks)
- x/evm/precompile/funtoken_test.go (4 hunks)
- x/evm/precompile/oracle.go (1 hunks)
- x/evm/precompile/oracle_test.go (2 hunks)
- x/evm/precompile/precompile.go (2 hunks)
- x/evm/precompile/test/export.go (1 hunks)
- x/evm/precompile/wasm.go (6 hunks)
- x/evm/precompile/wasm_test.go (8 hunks)
- x/evm/statedb/journal.go (12 hunks)
- x/evm/statedb/journal_test.go (1 hunks)
- x/evm/statedb/state_object.go (4 hunks)
- x/evm/statedb/statedb.go (11 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- x/evm/keeper/grpc_query.go
- x/evm/precompile/funtoken.go
- x/evm/precompile/oracle.go
- x/evm/precompile/oracle_test.go
- x/evm/precompile/wasm.go
- x/evm/statedb/journal.go
- x/evm/statedb/state_object.go
- x/evm/statedb/statedb.go
🧰 Additional context used
🔇 Additional comments (17)
x/evm/statedb/journal_test.go (2)
29-32: LGTM! Well-structured Transition type.The Transition struct is well-designed with clear function signatures for running transitions and performing assertions.
70-86: Test appears incomplete.The test sets up multiple state transitions but only executes the first one. Consider either:
- Executing all transitions in a loop, or
- Documenting why only the first transition is executed
Additionally, verify that this partial execution adequately tests the StateDB journal functionality as mentioned in the PR objectives.
x/evm/keeper/funtoken_from_coin.go (2)
81-84: Verify state handling in CallContractWithInputThe addition of a blank identifier suggests CallContractWithInput now returns an additional value, likely related to state management. While this change aligns with the PR's objective of improving StateDB management, we should ensure proper state handling.
Let's verify the CallContractWithInput implementation:
Line range hint
1-91: Verify state consistency after ERC20 deploymentGiven the PR's focus on fixing StateDB issues, we should ensure that the state remains consistent after ERC20 deployment, particularly:
- The contract deployment state is properly committed
- The nonce increment is properly handled
- The ERC20 address calculation remains valid
Let's verify the state handling:
✅ Verification successful
Let me gather more information about the contract deployment and state handling.
Based on the extensive code analysis, I can now provide a final response regarding the state consistency verification.
State handling in ERC20 deployment is properly implemented
The code analysis reveals that the state handling during ERC20 deployment is properly implemented with multiple safeguards:
The nonce handling is correct:
GetAccNonceis used to calculate the contract address before deployment- The nonce increment happens automatically through the EVM state transitions
The contract deployment state is properly managed:
CallContractWithInputis used withcommit=trueparameter- The StateDB changes are properly committed through the EVM execution path
- The state changes are tracked in the journal and committed atomically
The ERC20 address calculation remains valid:
- The address is pre-calculated using the current nonce
- The nonce increment and contract deployment happen in the same transaction
- The state changes are atomic through the StateDB commit mechanism
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for nonce handling patterns rg -A 5 'GetAccNonce|SetAccNonce' # Look for state commit patterns around contract deployment ast-grep --pattern 'func $$$CallContractWithInput$$$'Length of output: 2355
Script:
#!/bin/bash # Look for CallContractWithInput implementation rg -A 10 "CallContractWithInput.*\(" # Check for StateDB related code rg -A 5 "StateDB|CommitStateDB" # Look for contract deployment patterns ast-grep --pattern 'func $$$CallContract$$$'Length of output: 79701
x/evm/precompile/funtoken_test.go (4)
8-8: LGTM: Clear import alias for ethereum/go-ethereum/common packageThe rename from
commontogethcommonimproves code clarity by explicitly indicating the package's origin.
52-64: LGTM: Consistent updates to use renamed importThe changes correctly update all
HexToAddresscalls to use the renamedgethcommonimport while maintaining the original test logic and error scenarios.
140-142: LGTM: Robust balance comparison using string representationConverting to string representation before comparison is a safer approach for comparing SDK.Int values, avoiding potential issues with different numeric types.
129-136: LGTM: Enhanced contract call implementation with better error handlingThe switch to
evmtest.CallContractTximproves the test by adding VM error validation and aligns with the PR's goal of better state management.Let's verify the implementation of the new helper function:
✅ Verification successful
Let me gather more context about the implementation by checking the rest of the function and related dependencies.
Let me get one more piece of information to see how the transaction is executed after signing.
I now have enough information to verify the implementation and generate the final response.
Verified: CallContractTx implementation is robust and well-structured
The
evmtest.CallContractTxhelper function provides a comprehensive implementation that:
- Properly manages nonces from the state DB
- Handles gas estimation through
GenerateEthTxMsgAndSigner- Implements proper transaction signing
- Executes the transaction via
EvmKeeper.EthereumTx- Returns detailed response including VM errors
The switch to this helper in the test is indeed an improvement as it encapsulates all the necessary contract interaction steps with proper error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of CallContractTx rg -A 10 "func CallContractTx" "x/evm/evmtest/"Length of output: 673
Script:
#!/bin/bash # Get the full implementation of CallContractTx and related functions rg -A 20 "func CallContractTx" "x/evm/evmtest/" # Check for GenerateEthTxMsgAndSigner implementation rg -A 15 "func GenerateEthTxMsgAndSigner" "x/evm/evmtest/"Length of output: 2101
Script:
#!/bin/bash # Look for the rest of CallContractTx implementation rg -B 5 -A 15 "err = ethTxMsg.Sign\(gethSigner, krSigner\)" "x/evm/evmtest/"Length of output: 1986
x/evm/precompile/wasm_test.go (5)
14-14: LGTM: Import addition aligns with test package refactoring.The addition of the test package import is consistent with the refactoring effort to consolidate test utilities.
27-27: LGTM: Test refactoring maintains functionality while improving structure.The changes appropriately use the new test package utilities and follow the correct pattern for contract calls.
Also applies to: 55-58
96-105: Great improvement in test readability and maintainability!The refactoring effectively:
- Documents state transitions with clear comments
- Reduces code duplication through test utilities
- Makes the test flow more understandable
139-142: LGTM: Consistent error handling and type usage.The changes appropriately use the refactored response type and follow the established error handling pattern.
Also applies to: 162-164
55-58: Verify test coverage for StateDB commit behavior.While the tests cover various aspects of WASM precompile execution, consider adding specific test cases that verify the StateDB commit behavior at the start of precompiles, which is the main objective of this PR.
Would you like help in adding test cases specifically for verifying StateDB commit behavior?
Also applies to: 84-87, 139-142, 309-313
x/evm/precompile/precompile.go (1)
136-203: LGTM!The introduction of
OnRunStartResultandOnRunStartenhances the precompile execution flow. The implementation is clear and correctly handles input decomposition, context retrieval, and error handling.x/evm/keeper/erc20.go (1)
223-224: Duplicate assignment ofTxIndexandLogIndexmay cause inconsistenciesYou have assigned both
txConfig.TxIndexandtxConfig.LogIndextok.EvmState.BlockLogSize.GetOr(ctx, 0)at lines 223-224. As previously noted,TxIndexrepresents the index of the transaction within the block, whileLogIndexis the index of the log within the transaction. Using the same value for both can lead to inaccuracies.Please refer to the previous review comments for detailed guidance on this issue.
x/evm/keeper/msg_server.go (2)
Line range hint
266-401: Consistent error handling andevmObjusageThe
ApplyEvmMsgfunction now returnsevmObj *vm.EVMalong with the response and error. In error cases,evmObjmay not be fully initialized, which could lead to nil pointer dereferences if not handled carefully.Ensure that wherever
evmObjis used after callingApplyEvmMsg, it is checked fornilto prevent potential runtime panics.Check all usages of
evmObjafter callingApplyEvmMsgto ensure it is notnilbefore accessing its fields or methods.
81-82: Update to use consistent logging methodsEnsure that the usage of
k.EvmState.CalcBloomFromLogsaligns with the rest of the codebase and that the contextctxis appropriately passed.The code updates seem consistent with the overall codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (13)
x/evm/keeper/erc20_test.go (3)
19-22: Consider improving code formatting for better readabilityThe function call arguments could be formatted more consistently. Consider aligning the parameters vertically:
- _, _, err := deps.EvmKeeper.ERC20().Mint( - contract, deps.Sender.EthAddr, evm.EVM_MODULE_ADDRESS, - big.NewInt(69_420), deps.Ctx, - ) + _, _, err := deps.EvmKeeper.ERC20().Mint( + contract, + deps.Sender.EthAddr, + evm.EVM_MODULE_ADDRESS, + big.NewInt(69_420), + deps.Ctx, + )
28-28: Split long line for better readabilityThe function call is quite long and could be split across multiple lines for better readability:
- _, _, err := deps.EvmKeeper.ERC20().Mint(contract, evm.EVM_MODULE_ADDRESS, evm.EVM_MODULE_ADDRESS, big.NewInt(69_420), deps.Ctx) + _, _, err := deps.EvmKeeper.ERC20().Mint( + contract, + evm.EVM_MODULE_ADDRESS, + evm.EVM_MODULE_ADDRESS, + big.NewInt(69_420), + deps.Ctx, + )
37-37: Split long line for better readabilityThe function call could be split across multiple lines:
- _, _, err := deps.EvmKeeper.ERC20().Transfer(contract, deps.Sender.EthAddr, evm.EVM_MODULE_ADDRESS, big.NewInt(9_420), deps.Ctx) + _, _, err := deps.EvmKeeper.ERC20().Transfer( + contract, + deps.Sender.EthAddr, + evm.EVM_MODULE_ADDRESS, + big.NewInt(9_420), + deps.Ctx, + )x/evm/evmmodule/genesis_test.go (1)
57-57: LGTM! Consider documenting the ignored return values.The changes correctly handle the updated
Transfermethod signature. For better clarity, consider adding a comment explaining what the ignored return values represent.Example improvement:
- _, _, err = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserA, amountToSendA, deps.Ctx) + // Ignoring evmObj and result returns as they're not needed for this test + _, _, err = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserA, amountToSendA, deps.Ctx)Also applies to: 61-61
x/evm/precompile/funtoken.go (1)
Line range hint
177-177: Address TODO: Implement EVM event emission.Event emission is important for transaction transparency and off-chain monitoring.
Would you like me to help implement the EVM event emission for these operations?
x/evm/precompile/precompile.go (2)
137-150: Well-designed struct with clear separation of concerns.The
OnRunStartResultstruct effectively encapsulates all necessary components for precompile execution, providing a clean interface for state management. The clear documentation and strong typing will help prevent state-related bugs.Consider adding validation methods to this struct to ensure state consistency, particularly for the
StateDBandCtxfields which are critical for maintaining system integrity.
152-178: Enhance documentation with error scenarios.While the function documentation is comprehensive, it would be beneficial to document the possible error scenarios that callers should handle.
Add error scenarios to the documentation:
// Args: // - evm: Instance of the EVM executing the contract // - contract: Precompiled contract being called // - abi: [gethabi.ABI] defining the contract's invokable methods. +// +// Returns error when: +// - Input data cannot be decoded according to the ABI +// - StateDB type assertion fails +// - Committing dirty journal entries failsx/evm/precompile/wasm_test.go (1)
309-312: Consider enhancing error message with more contextWhile the error handling is good, consider making the error message more descriptive by including the test case name and input parameters.
-s.Require().ErrorContains(err, tc.wantError, "ethTxResp %v", ethTxResp) +s.Require().ErrorContains(err, tc.wantError, "test case '%s' failed with ethTxResp %v", tc.name, ethTxResp)x/evm/keeper/msg_server.go (5)
275-275: Standardize error wrapping pattern.The error wrapping pattern is inconsistent between these returns. Line 275 uses
errors.Wrap(err, "intrinsic gas failed")while lines 286-290 use a different format witherrors.Wrapf(core.ErrIntrinsicGas, "apply message msg.Gas = %d, intrinsic gas = %d.", leftoverGas, intrinsicGas). Consider standardizing the error wrapping pattern for better consistency.Also applies to: 286-290
355-357: Align error wrapping with project pattern.Consider using
errors.Wrapinstead offmt.Errorfto maintain consistency with the project's error handling pattern:- return nil, evmObj, fmt.Errorf("failed to commit stateDB: %w", err) + return nil, evmObj, errors.Wrap(err, "failed to commit stateDB")
Line range hint
689-694: Add validation for contract deployment address.The contract address is generated but not validated before event emission. Consider adding validation to ensure the generated contract address is valid:
if recipient == nil { // contract creation contractAddr := crypto.CreateAddress(msg.From(), msg.Nonce()) + if !gethcommon.IsHexAddress(contractAddr.Hex()) { + return errors.Wrap(err, "invalid contract address generated") + } _ = ctx.EventManager().EmitTypedEvent(&evm.EventContractDeployed{ Sender: msg.From().Hex(), ContractAddr: contractAddr.String(), })
364-368: Enhance gas overflow error messages.The error messages for gas overflow could be more descriptive to aid debugging. Consider adding more context:
- return nil, evmObj, errors.Wrapf(evm.ErrGasOverflow, "minimumGasUsed(%s) is not a uint64", minimumGasUsed.TruncateInt().String()) + return nil, evmObj, errors.Wrapf(evm.ErrGasOverflow, "minimum gas used (%s) exceeds uint64 max value", minimumGasUsed.TruncateInt().String()) - return nil, evmObj, errors.Wrapf(evm.ErrGasOverflow, "message gas limit < leftover gas (%d < %d)", msg.Gas(), leftoverGas) + return nil, evmObj, errors.Wrapf(evm.ErrGasOverflow, "gas limit underflow: message gas limit (%d) is less than leftover gas (%d)", msg.Gas(), leftoverGas)
Line range hint
593-603: Add pre-transfer balance validation.Consider adding a validation check for the EVM module's balance before attempting the transfer to fail fast and provide a clearer error message:
+ if evmModuleBalance.Cmp(coin.Amount.BigInt()) < 0 { + return nil, errors.Wrapf(err, "insufficient balance in EVM module: has %s, needs %s", evmModuleBalance, coin.Amount) + } res, _, err := k.ERC20().Transfer( erc20Addr, evm.EVM_MODULE_ADDRESS, recipient, coin.Amount.BigInt(), ctx, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- CHANGELOG.md (3 hunks)
- x/evm/evmmodule/genesis_test.go (1 hunks)
- x/evm/keeper/erc20.go (7 hunks)
- x/evm/keeper/erc20_test.go (3 hunks)
- x/evm/keeper/funtoken_from_erc20_test.go (5 hunks)
- x/evm/keeper/msg_server.go (11 hunks)
- x/evm/precompile/funtoken.go (4 hunks)
- x/evm/precompile/precompile.go (2 hunks)
- x/evm/precompile/wasm_test.go (8 hunks)
- x/evm/statedb/journal.go (12 hunks)
- x/evm/statedb/journal_test.go (1 hunks)
- x/evm/statedb/state_object.go (8 hunks)
- x/evm/statedb/statedb.go (13 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- x/evm/keeper/funtoken_from_erc20_test.go
- x/evm/statedb/journal.go
- x/evm/statedb/journal_test.go
- x/evm/statedb/statedb.go
🔇 Additional comments (29)
x/evm/keeper/erc20_test.go (2)
Line range hint
1-63: Add test cases for StateDB commit behaviorWhile the test file covers basic ERC20 operations, it doesn't explicitly verify the PR's main objective of fixing issues with uncommitted, dirty journal entries in the StateDB. Consider adding test cases that:
- Verify the StateDB is committed at precompile start
- Demonstrate that the EVM and non-EVM state remain in sync after operations
- Test scenarios where operations revert to ensure proper state handling
This aligns with the pending item in the PR checklist regarding "test case that reverts the StateDB".
#!/bin/bash # Description: Check for existing StateDB commit/revert tests # Search for StateDB-related test cases rg -B 2 -A 2 "StateDB.*(?:Commit|Revert)" --type go # Search for test cases involving state synchronization rg -B 2 -A 2 "(?i)state.*sync" --type go
46-46: Split long line and verify consistent return value handlingThe function call could be split across multiple lines for better readability. Additionally, we should verify that the new return value (evmObj) is consistently handled across all ERC20 operations.
- _, _, err := deps.EvmKeeper.ERC20().Transfer(contract, evm.EVM_MODULE_ADDRESS, deps.Sender.EthAddr, big.NewInt(9_420), deps.Ctx) + _, _, err := deps.EvmKeeper.ERC20().Transfer( + contract, + evm.EVM_MODULE_ADDRESS, + deps.Sender.EthAddr, + big.NewInt(9_420), + deps.Ctx, + )x/evm/precompile/funtoken.go (5)
32-34: LGTM: Clean ABI getter implementation.The new
ABI()method provides a clean way to access the contract ABI, improving code organization.
37-39: LGTM: Improved gas calculation.The change to use a shared
RequiredGasimplementation with ABI parameter suggests more accurate gas calculation.
51-53: LGTM: Improved error handling.The error handling is now more consistent using
ErrPrecompileRun, which should provide better error context.
72-73: Verify StateDB commitment behavior.The addition of
StateDB.Commit()addresses the PR objective of fixing uncommitted journal entries. However, we should verify this doesn't affect other precompiles.✅ Verification successful
Let me try a different search pattern to find precompile implementations.
The first command will help identify precompile structs and their methods, while the second will show where StateDB commits are being used in the codebase. This will give us better context about the commit behavior across precompiles.
Let me search for more specific context about precompile implementations.
This will help us understand the precompile architecture and how StateDB.Commit() is being used in the context of the precompile framework.
Based on the code analysis, I can now provide a final response about the StateDB commitment behavior.
StateDB.Commit() usage is consistent with precompile framework design
The addition of
StateDB.Commit()in the funtoken precompile aligns with the framework's design and other precompiles. Key observations:
- The
precompile.gofile explicitly documents inOnRunStart()that precompiles should "Use res.StateDB.Commit() before any non-EVM state changes to guarantee the context and [statedb.StateDB] are in sync."- Both funtoken and wasm precompiles follow this pattern by calling
start.StateDB.Commit()at the end of theirRun()methods- This ensures that any EVM state changes are properly committed before making Cosmos SDK state modifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if other precompiles also commit their state echo "Searching for other precompile Run methods that might be affected by StateDB commits..." rg -A 5 "Run\(.*\*vm\.EVM.*\*vm\.Contract" --type goLength of output: 239
Script:
#!/bin/bash # Search for precompile Run methods with different pattern rg -A 10 "type.*Precompile.*struct" --type go # Also search for StateDB.Commit() usage rg "StateDB\.Commit\(\)" --type goLength of output: 552
Script:
#!/bin/bash # Look at the precompile.go file to understand the base implementation cat x/evm/precompile/precompile.go # Look at both funtoken and wasm precompile implementations cat x/evm/precompile/funtoken.go cat x/evm/precompile/wasm.goLength of output: 23164
144-147: Verify ERC20 transfer error handling.The ERC20 transfer is a critical operation. Ensure proper error handling and state rollback on failure.
✅ Verification successful
Let me try a different pattern to locate the ERC20 transfer implementation.
The previous pattern was too specific. Let's try a broader search to find the relevant code and error handling patterns. This will help us verify the error handling and state rollback mechanisms in place.
Let me try one more search with a different pattern to locate the ERC20 implementation.
I'm broadening the search to first find files containing ERC20 references, then examining their content for transfer-related implementations. This should help us locate the relevant code to verify the error handling mechanisms.
Let me check the specific implementation of the Transfer function and its error handling.
This should help us verify the error handling and state management in the ERC20 transfer implementation.
Let me try one final search with a different pattern to locate the transfer implementation.
This broader search should help us locate the transfer implementation and its error handling patterns across the relevant files.
Based on the code revealed, I can now provide a final response about the ERC20 transfer error handling:
ERC20 transfer implementation has proper error handling in place
The implementation shows appropriate error handling at multiple levels:
- Input validation through ABI packing
- Contract call error handling
- Response unpacking validation
- All errors are properly propagated with context
The transfer operation is atomic through the EVM transaction mechanism, which ensures state rollback on failure. The implementation in
erc20.goshows proper error handling patterns with clear error wrapping and propagation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check ERC20 transfer implementation and error handling echo "Searching for ERC20 transfer implementation..." ast-grep --pattern 'func $_($_ *keeper.$_, $_ gethcommon.Address, $_ gethcommon.Address, $_ *big.Int, $_ sdk.Context) ($_, $_, error)'Length of output: 239
Script:
#!/bin/bash # Search for ERC20 transfer related code echo "Searching for ERC20 transfer implementation..." rg -A 10 "func.*Transfer.*ERC20" # Also check for any error handling patterns echo -e "\nChecking error handling patterns..." rg "if err.*transfer" -A 3Length of output: 253
Script:
#!/bin/bash # Search for ERC20 interface and implementation echo "Searching for ERC20 related code..." rg -l "ERC20" # Then let's examine the content of files that mention ERC20 echo -e "\nChecking specific ERC20 files..." fd -t f -e go . | xargs rg "Transfer.*ERC20|func.*ERC20" -A 5Length of output: 11125
Script:
#!/bin/bash # Look for the Transfer implementation in erc20.go echo "Checking Transfer implementation..." rg -A 15 "func.*Transfer.*implements.*ERC20.transfer" x/evm/keeper/erc20.go # Check for state management and rollback mechanisms echo -e "\nChecking state management..." rg "StateDB|stateDB" x/evm/keeper/erc20.go -A 3Length of output: 281
Script:
#!/bin/bash # Look for Transfer method in erc20.go with more context echo "Checking Transfer method..." rg -B 2 -A 20 "func \(.*\) Transfer" x/evm/keeper/erc20.go # Also check for any transaction handling patterns echo -e "\nChecking transaction patterns..." rg "func \(k \*?Keeper\)" x/evm/keeper/erc20.go -A 3 # Check for any error handling in the funtoken.go where the transfer is called echo -e "\nChecking funtoken.go error handling..." rg "Transfer.*err.*return" x/evm/precompile/funtoken.go -A 5Length of output: 2215
x/evm/precompile/precompile.go (2)
27-30: LGTM: Import changes are appropriate.The new imports are necessary for the added functionality and are used effectively throughout the code.
192-194: Verify state consistency after commit.The commit operation is crucial for maintaining state consistency. Consider adding verification to ensure the commit was successful and the state is as expected.
x/evm/precompile/wasm_test.go (6)
14-14: LGTM: Good modularization of test utilitiesThe addition of a dedicated test package helps improve code organization and reusability.
55-57: LGTM: Improved error handling patternThe contract calls now follow a consistent pattern with explicit error handling and response validation. This makes the test behavior more predictable and easier to debug.
Also applies to: 84-86
99-111: Great improvement in test readabilityThe test flow is now more explicit with:
- Clear state transition comments
- Step-by-step verification
- Consistent use of test utilities
139-142: LGTM: Consistent error handlingThe response handling follows the established pattern with proper error checking and response validation.
162-165: LGTM: Enhanced response validationGood addition of owner validation alongside the count check, improving test coverage.
Line range hint
1-315: Consider adding StateDB commit verification testsGiven that the PR objectives mention fixing issues with uncommitted, dirty journal entries in the EVM StateDB, consider adding specific test cases that verify:
- The temporary state is correctly committed at precompile start
- The state remains consistent after precompile execution
- The behavior during precompile reversion
This would help ensure the core issue is properly addressed and prevent regressions.
x/evm/statedb/state_object.go (6)
141-142: LGTM: Storage initialization is correct.The initialization of
OriginStorageandDirtyStorageas new maps is properly maintained.
173-175: Improved clarity in balance tracking.The rename from
prevtoprevWeibetter indicates that the value is in wei units, consistent with theAccountWeistruct design.
259-264: LGTM: Storage access logic is preserved.The state access and modification logic remains correct after the field renames. The proper order of checking dirty state before committed state is maintained.
Let's verify no unintended changes to storage access patterns:
#!/bin/bash # Check storage access patterns rg "Get(?:Committed)?State|setState" --type goAlso applies to: 270-273, 293-293
118-119: Consider keeping storage fields private to maintain encapsulation.The change from private (
originStorage,dirtyStorage) to public (OriginStorage,DirtyStorage) visibility could allow external components to modify storage state directly, potentially bypassing journal entries. This might lead to inconsistent state or break atomicity guarantees.Let's check if these fields are accessed from outside the package:
#!/bin/bash # Search for direct access to OriginStorage or DirtyStorage fields rg "\.(?:Origin|Dirty)Storage\b" --type goConsider:
- Keeping these fields private
- Providing controlled access through methods that maintain proper journaling
- If external access is required, document the thread-safety and atomicity guarantees
Line range hint
215-219: Verify journal visibility requirements.The change from
journaltoJournalacross multiple methods suggests the journal needs to be accessed from outside the package. This could impact the atomicity of state changes.Let's check the journal usage pattern:
#!/bin/bash # Search for Journal access patterns rg "\.Journal\." --type goConsider documenting:
- Why the journal needs to be public
- Thread-safety guarantees
- Impact on state atomicity
Also applies to: 231-234, 284-288
124-125:⚠️ Potential issueProtect critical flags from external modification.
Making
DirtyCodeandSuicidedflags public could lead to security vulnerabilities. TheSuicidedflag is particularly sensitive as it determines if an account should be deleted during state updates.Let's verify the usage of these flags:
Consider keeping these flags private and providing controlled access through methods that ensure proper state transitions and journaling.
x/evm/keeper/erc20.go (5)
19-19: LGTM: Import addition aligns with PR objectivesThe addition of the statedb package import is consistent with the PR's goal of fixing StateDB-related issues.
55-58: Improved state tracking with EVM object return valueThe addition of
evmObj *vm.EVMas a return value in Mint and Transfer methods enables better state tracking and aligns with the PR's goal of fixing StateDB issues. The error wrapping improvements also enhance debugging capabilities.Also applies to: 76-79, 81-83, 89-92
213-214: Enhanced error handling with better contextThe error handling has been significantly improved with:
- More specific error messages for different failure scenarios
- Better context through error wrapping
- Proper handling of VM-specific errors (OutOfGas, ExecutionReverted)
This makes debugging easier and provides better visibility into failure modes.
Also applies to: 233-234, 240-241, 243-244, 246-247, 255-255, 261-261
219-222: Improved transaction configuration with proper indicesThe changes correctly initialize transaction configuration:
- Block hash is properly generated from context header
- TxIndex now correctly uses BlockTxIndex instead of BlockLogSize
- LogIndex is properly tracked
227-229: Verify state synchronization in edge casesThe state management looks correct, but we should verify the synchronization in edge cases:
- When a transaction partially succeeds but later reverts
- When multiple transactions are processed in the same block
Also applies to: 264-266
✅ Verification successful
Let me try a different search strategy to find relevant test cases and state management patterns.
Let's analyze the test coverage and state management patterns to verify the edge cases mentioned in the review comment.
Based on the search results, I can now provide a conclusive response about the state synchronization concerns in the review comment.
State synchronization is properly handled for transaction reverts and multiple transactions
The codebase shows robust state management for transaction reverts and multiple transactions:
- The code uses a temporary context (
tmpCtx, commitCtx := ctx.CacheContext()) to isolate state changes during EVM execution- The EVM state management system has comprehensive revert handling:
RevertToSnapshotfunctionality in StateDB- Journal system to track and revert state changes
- Proper handling of reverted transactions through
vm.ErrExecutionReverted- Multiple transaction handling is safe because:
- Each transaction uses an isolated cached context
- State changes are only committed if the transaction succeeds
- The journal system ensures atomic state updates
The implementation follows best practices for state management in EVM transactions, making it safe for both reverted transactions and multiple transaction scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases that verify state synchronization in edge cases rg -A 10 "TestCase.*Revert" --type go rg -A 10 "TestCase.*MultipleTransactions" --type go # Search for potential state management patterns ast-grep --pattern 'func TestCase$_($_, $_) { $$$ ctx.$_( $$$ ) $$$ }'Length of output: 178
Script:
#!/bin/bash # Search for test cases related to EVM transaction handling and state management rg -A 10 "Test.*EVM.*Transaction" --type go rg -A 10 "Test.*State.*" --type go # Look for revert handling patterns rg -A 5 "revert" --type go # Search for specific EVM state management functions ast-grep --pattern 'func ($_ *Keeper) ApplyEvmMsg($$$) { $$$ }' # Look for context handling in tests rg -A 5 "WithContext|NewContext" --type goLength of output: 81835
CHANGELOG.md (3)
53-59: Critical fix for EVM precompile state management.The changes in PR #2086 address an important issue where the EVM precompile could retrieve stale state due to uncommitted journal entries. This fix ensures proper state synchronization between EVM and non-EVM contexts.
43-61: Well-organized changelog section for Nibiru EVM.The changelog entries are properly formatted with:
- Clear PR links
- Descriptive summaries
- Proper categorization of changes
143-151: Breaking changes are clearly documented.The State Machine Breaking section properly documents significant changes:
- Removal of wasmbinding Custom bindings
- Changes to inflation parameters
- Migration to cosmossdk.io/math
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
x/evm/statedb/journal_test.go (2)
28-29: Consider defining magic numbers as constants.The funding amount
69_420should be defined as a constant at the package level for better maintainability and reusability.+const ( + testFundingAmount = 69_420 +) s.Require().NoError(testapp.FundAccount( deps.App.BankKeeper, deps.Ctx, deps.Sender.NibiruAddr, - sdk.NewCoins(sdk.NewCoin(bankDenom, sdk.NewInt(69_420))), + sdk.NewCoins(sdk.NewCoin(bankDenom, sdk.NewInt(testFundingAmount))), ))
76-81: Enhance test documentation for state changes.The comments could be more descriptive about why specific numbers of dirties are expected. Consider adding more detailed explanations.
-// 2 dirties from [createObjectChange, balanceChange] +// Expect 2 dirties: 1 from creating new account (createObjectChange) + 1 from balance modification (balanceChange) stateDB.AddBalance(randomAcc, balDelta) -// 1 dirties from [balanceChange] +// Expect 1 dirty: only balance modification (balanceChange) as account already exists stateDB.AddBalance(randomAcc, balDelta) -// 1 dirties from [balanceChange] +// Expect 1 dirty: only balance modification (balanceChange) stateDB.SubBalance(randomAcc, balDelta)x/evm/precompile/funtoken.go (2)
Line range hint
190-190: Address the TODO comment for EVM eventsThe TODO comment indicates that EVM events should be emitted. This is important for transaction tracking and UI integration.
Would you like me to help implement the EVM event emission for these operations?
194-244: LGTM: Well-structured helper functions with proper state synchronizationThe new helper functions properly encapsulate common operations and maintain consistency between bank and EVM balances. The special handling of EVMBankDenom ensures proper state synchronization.
Consider adding documentation comments to explain:
- When StateDB balance updates are necessary
- The relationship between bank balances and EVM balances
- Why EVMBankDenom requires special handling
+// SafeMintCoins mints coins to a module account and updates the StateDB balance +// if the denomination is EVMBankDenom. This ensures consistency between +// Cosmos SDK bank balances and EVM account balances. func SafeMintCoins( ctx sdk.Context, moduleName string, amt sdk.Coin, bk bankkeeper.Keeper, db *statedb.StateDB, ) error { +// SafeSendCoinFromModuleToAccount sends coins from a module account to a user account +// and updates the StateDB balances if the denomination is EVMBankDenom. This ensures +// consistency between Cosmos SDK bank balances and EVM account balances for both +// the sender and recipient. func SafeSendCoinFromModuleToAccount( ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coin, bk bankkeeper.Keeper, db *statedb.StateDB, ) error {x/evm/statedb/statedb.go (1)
301-301: Ensure parameter naming consistency for clarityIn the
AddBalancemethod, the parameter name has been changed fromamounttowei. Ensure that this naming convention is consistent throughout the codebase to improve readability and indicate that balances are handled in Wei units.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- x/evm/precompile/funtoken.go (8 hunks)
- x/evm/statedb/journal.go (12 hunks)
- x/evm/statedb/journal_test.go (1 hunks)
- x/evm/statedb/statedb.go (14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/statedb/journal.go
🔇 Additional comments (4)
x/evm/statedb/journal_test.go (1)
21-147: Well-structured test suite that aligns with PR objectives.The test suite effectively validates the StateDB journal functionality with comprehensive coverage of different scenarios:
- Basic state transitions
- Contract interactions
- Precompile calls
- Error cases
The tests align well with the PR objective of fixing issues with uncommitted dirty journal entries.
x/evm/precompile/funtoken.go (3)
10-10: LGTM: Clean import changes and ABI method additionThe new imports and ABI method implementation look good. The ABI method provides a clean way to access the contract's interface definition.
Also applies to: 35-37
146-151: Verify ERC20 transfer error handlingThe error handling for ERC20 transfer has been improved, but we should verify that failed transfers don't leave the state in an inconsistent state.
#!/bin/bash # Check if there are any state modifications before the ERC20 transfer ast-grep --pattern 'func ($x *$type) bankSend($_, $_, $_) ($_, $_) { $$$ $keeper.ERC20().Transfer($$$) }'
54-76: LGTM: Improved error handling and StateDB commitThe changes properly address the issue with uncommitted journal entries by:
- Simplifying error handling with
ErrPrecompileRun- Using
OnRunStartfor consistent state management- Adding
StateDB.Commit()to ensure temporary state is committedLet's verify the StateDB commit behavior:
✅ Verification successful
Let me try a different approach to verify the StateDB commit behavior.
This should help us see the Run method implementations and verify the commit behavior across precompiles.
Let me try one final verification to specifically look at other precompile Run methods.
This should help us see the complete Run method implementations in precompiles to verify the commit behavior consistency.
Let me try one last search with a modified pattern.
This should help us find the Run method implementations in precompile files, regardless of the exact parameter names.
Based on the search results, I can now provide a final response:
StateDB.Commit() is correctly implemented across precompiles
The verification confirms that:
- All precompile Run methods (wasm.go, oracle.go, funtoken.go) use the
OnRunStarthelper- The
OnRunStarthelper ensures proper StateDB commit handling- There's clear documentation in precompile.go showing this is the recommended pattern
- The commit is properly handled both in the helper and at the end of precompile executions
The changes in funtoken.go follow the established pattern and are consistent with other precompiles in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if StateDB.Commit() is consistently called across other precompiles ast-grep --pattern 'func ($x *$type) Run($_, $_, $_) ($_, $_) { $$$ return $_, $statedb.Commit() }'Length of output: 111
Script:
#!/bin/bash # Search for Run methods in precompile implementations rg "func.*Run\(.*\).*\{" -A 5 # Also check for Commit calls rg "StateDB.*Commit\(\)" -B 3Length of output: 3845
Script:
#!/bin/bash # Search for all precompile Run method implementations rg "func \(.*\) Run\(.*evm.*contract.*readonly.*\)" -A 10 x/evm/precompile/Length of output: 77
Script:
#!/bin/bash # Search for Run methods in precompile files rg "func.*Run.*\(" --type go -g "x/evm/precompile/*.go" -B 1 -A 15Length of output: 5832
Unique-Divine
deleted the
ud/evm-fix-state-not-committed-before-precompile
branch
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
x/evm/statedb/journal_test.go (1)
113-116: Consider enhancing error message clarity.The error message could be more descriptive by including the actual dirty count found.
- s.FailNow("expected 2 dirty journal changes") + s.FailNow(fmt.Sprintf("expected 2 dirty journal changes, got %d", stateDB.DirtiesCount()))x/evm/statedb/statedb_test.go (1)
516-521: LGTM! Consider enhancing test readability.The change to use struct literal syntax for TxConfig initialization improves code clarity by making the configuration fields explicit. This aligns well with the PR's objective of addressing StateDB journal entry issues.
Consider adding a brief comment explaining the purpose of these non-default test values:
const ( + // Using non-default values to ensure proper initialization blockNumber = uint64(1) txIdx = uint(1) logIdx = uint(1) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- x/evm/keeper/erc20.go (7 hunks)
- x/evm/keeper/erc20_test.go (1 hunks)
- x/evm/keeper/msg_server.go (10 hunks)
- x/evm/keeper/vm_config.go (1 hunks)
- x/evm/precompile/funtoken.go (8 hunks)
- x/evm/statedb/config.go (1 hunks)
- x/evm/statedb/journal_test.go (1 hunks)
- x/evm/statedb/statedb_test.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/evm/keeper/erc20_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/precompile/funtoken.go
🔇 Additional comments (14)
x/evm/statedb/config.go (2)
23-25: LGTM! Parameter name change improves clarity.The rename from
bhashtoblockHashenhances readability and maintains consistency with the struct field name.
Line range hint
23-29: Verify the removal of NewTxConfig function.The removal of
NewTxConfigfunction is a breaking change. Let's verify that all callers have been updated to use alternative methods.✅ Verification successful
The search results show references to a different
NewTxConfigfunction from the Cosmos SDK'stxpackage (tx.NewTxConfig), which is unrelated to theNewTxConfigfunction being removed fromx/evm/statedb/config.go. Let me verify this further.
Let me do one final check to see if there are any references to this function in the test files within the EVM module, as tests are often the primary consumers of such utility functions.
Let me fix the regex syntax error and try one last search.
Based on all the searches performed, I can now provide a final response:
The removal of
NewTxConfigappears to be safeThe extensive searches through the codebase have shown:
- No direct calls to the removed
NewTxConfigfunction in the EVM module- The only
NewTxConfigreferences found were to a different function from Cosmos SDK's tx package- No imports or usages of the statedb package that would indicate dependency on this function
The function appears to have been unused and its removal is replaced by the new
NewEmptyTxConfigfunction, which provides a simpler interface for creating a TxConfig with just a block hash parameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining references to NewTxConfig that might need updating # Test 1: Look for direct function calls echo "Searching for NewTxConfig function calls..." rg "NewTxConfig\(" # Test 2: Look for any references in tests echo "Searching for NewTxConfig in test files..." rg "NewTxConfig" -g "*_test.go" # Test 3: Look for any comments or documentation mentioning the function echo "Searching for NewTxConfig in comments and docs..." rg "NewTxConfig" -g "*.md"Length of output: 627
Script:
#!/bin/bash # Let's verify this by: # 1. Looking specifically in the evm package for any references # 2. Looking for the exact function signature with ast-grep # Search in evm package echo "Searching in evm package..." rg "NewTxConfig" x/evm/ # Search for the specific function signature echo "Searching for the exact function signature..." ast-grep --pattern 'func NewTxConfig(blockHash $_, txHash $_, txIndex $_, logIndex $_) TxConfig'Length of output: 288
Script:
#!/bin/bash # Search specifically in EVM test files echo "Searching in EVM test files..." rg "NewTxConfig" x/evm/ -g "*_test.go" # Also check for any uses of TxConfig struct initialization that might have been using this function echo "Searching for TxConfig struct usage..." rg "TxConfig{" x/evm/Length of output: 302
Script:
#!/bin/bash # Search for TxConfig struct initialization with proper regex escaping echo "Searching for TxConfig struct usage..." rg "TxConfig\s*{" x/evm/ # Also look for any imports of the statedb package which might indicate usage echo "Searching for statedb imports..." rg '"x/evm/statedb"' x/evm/Length of output: 306
x/evm/keeper/vm_config.go (1)
42-47: LGTM! The refactored TxConfig implementation improves clarity and state management.The change from using a constructor to a direct struct initialization makes the source of each field value more explicit and ensures proper state access through the keeper.
Let's verify the state access methods being used:
✅ Verification successful
Let me try a different approach to verify the state access methods.
Let me search specifically for the collections package and GetOr method since it appears to be from a custom collections implementation.
Let me try one more search to find the ItemTransient implementation.
Based on the gathered information, I can now generate the final response. The code shows that
BlockTxIndexandBlockLogSizeare part of theEvmStatestruct and are implemented ascollections.ItemTransientfrom thegithub.com/NibiruChain/collectionspackage. TheGetOrmethod is consistently used throughout the codebase to safely retrieve values with a default fallback, and the usage invm_config.gofollows this pattern correctly.The TxConfig implementation correctly uses transient state values for indices
The code properly retrieves transaction and log indices from the EVM state using the
GetOrmethod, which safely returns a default value of 0 if no value is set. This is the expected behavior for tracking block-level transaction and log indices.
BlockTxIndexandBlockLogSizeare correctly defined as transient state items inx/evm/keeper/evm_state.go- The
GetOrmethod is consistently used across the codebase for safe value retrieval with defaults- The usage pattern matches other parts of the codebase, such as in
msg_server.goandevmante_emit_event.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and usage of EvmState methods # Test 1: Check for BlockTxIndex definition and usage rg -A 2 "BlockTxIndex.*=.*struct" # Test 2: Check for BlockLogSize definition and usage rg -A 2 "BlockLogSize.*=.*struct" # Test 3: Check for GetOr method implementation ast-grep --pattern 'func $_GetOr(ctx $_, $default $_) $_'Length of output: 131
Script:
#!/bin/bash # Search for EvmState type definition and its fields rg -A 10 "type EvmState struct" # Search for any usage of BlockTxIndex and BlockLogSize rg "BlockTxIndex|BlockLogSize" # Look for GetOr method implementations in the codebase rg -A 3 "func.*GetOr" # Check the imports in vm_config.go to verify statedb package rg -A 10 "^import \(" "x/evm/keeper/vm_config.go"Length of output: 2459
Script:
#!/bin/bash # Search for collections package import and usage rg "collections\." -A 2 # Look for ItemTransient type and its methods rg "type ItemTransient" # Check if collections is defined in the local codebase fd collections.go # Search for GetOr method in collections related files rg "func.*GetOr" --type goLength of output: 33363
Script:
#!/bin/bash # Search for ItemTransient in go.mod dependencies cat go.mod # Look for collections package imports rg "\"collections\"" --type go # Search for GetOr usage in the codebase rg "\.GetOr\(" -B 2 -A 2Length of output: 20326
x/evm/statedb/journal_test.go (1)
72-95: Well-structured test for journal entry management.This test case effectively validates the core functionality of managing dirty journal entries and their cleanup after commit, which directly addresses the PR's objective of fixing issues with uncommitted, dirty journal entries.
x/evm/keeper/erc20.go (7)
59-60: LGTM: Appropriate handling of return valuesThe changes correctly handle the return values from
CallContractWithInput, discarding theevmObjwhich is only needed for testing purposes.
121-122: LGTM: Consistent with Mint function implementationThe changes maintain consistency with the Mint function implementation, appropriately handling return values and error cases.
153-154: LGTM: Consistent error handling patternThe changes maintain consistency with the established pattern of handling return values and errors.
177-177: LGTM: Enhanced return values for better debuggingThe addition of
evmObj *vm.EVMas a return value improves debugging capabilities and aligns with the PR objectives.
213-214: LGTM: Improved error handling and messagesThe changes enhance error handling by:
- Adding more context to wrapped errors
- Providing specific error messages for different VM error cases
- Making error messages more descriptive for debugging
Also applies to: 230-231, 237-238, 240-241, 243-244
252-252: LGTM: Enhanced error handling in success pathThe changes improve error handling in the success path by adding more context to errors related to gas usage and event emission.
Also applies to: 258-258
263-263: LGTM: Proper return value handlingThe return statement correctly handles all return values in the success case.
x/evm/keeper/msg_server.go (3)
65-65: LGTM: Updated function call to match new signature.The change correctly handles the additional return value from
ApplyEvmMsg.
249-249: LGTM: Enhanced error context with EVM object.The changes improve error handling by providing EVM context in error returns and initializing the EVM object earlier in the function flow.
Also applies to: 256-256
Line range hint
689-694: LGTM: Proper contract address calculation in event.The contract address calculation follows the Ethereum specification using
crypto.CreateAddress(from, nonce).
| s.T().Log("Set up helloworldcounter.wasm") | ||
|
|
||
| wasmContract := test.SetupWasmContracts(&deps, &s.Suite)[1] | ||
| fmt.Printf("wasmContract: %s\n", wasmContract) |
There was a problem hiding this comment.
Remove debug print statement.
Debug print statements should be removed from production code.
- fmt.Printf("wasmContract: %s\n", wasmContract)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt.Printf("wasmContract: %s\n", wasmContract) |
| import ( | ||
| "fmt" | ||
| "math/big" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| "github.com/ethereum/go-ethereum/core/vm" | ||
|
|
||
| serverconfig "github.com/NibiruChain/nibiru/v2/app/server/config" | ||
| "github.com/NibiruChain/nibiru/v2/x/common/testutil/testapp" | ||
| "github.com/NibiruChain/nibiru/v2/x/evm" | ||
| "github.com/NibiruChain/nibiru/v2/x/evm/embeds" | ||
| "github.com/NibiruChain/nibiru/v2/x/evm/evmtest" | ||
| "github.com/NibiruChain/nibiru/v2/x/evm/precompile/test" | ||
| "github.com/NibiruChain/nibiru/v2/x/evm/statedb" | ||
| ) |
There was a problem hiding this comment.
Remove duplicate import of test package.
The test package is imported twice - once with an alias (as seen in the code usage) and once without. This is redundant.
- "github.com/NibiruChain/nibiru/v2/x/evm/precompile/test"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import ( | |
| "fmt" | |
| "math/big" | |
| "strings" | |
| "testing" | |
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| "github.com/ethereum/go-ethereum/core/vm" | |
| serverconfig "github.com/NibiruChain/nibiru/v2/app/server/config" | |
| "github.com/NibiruChain/nibiru/v2/x/common/testutil/testapp" | |
| "github.com/NibiruChain/nibiru/v2/x/evm" | |
| "github.com/NibiruChain/nibiru/v2/x/evm/embeds" | |
| "github.com/NibiruChain/nibiru/v2/x/evm/evmtest" | |
| "github.com/NibiruChain/nibiru/v2/x/evm/precompile/test" | |
| "github.com/NibiruChain/nibiru/v2/x/evm/statedb" | |
| ) | |
| import ( | |
| "fmt" | |
| "math/big" | |
| "strings" | |
| "testing" | |
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| "github.com/ethereum/go-ethereum/core/vm" | |
| serverconfig "github.com/NibiruChain/nibiru/v2/app/server/config" | |
| "github.com/NibiruChain/nibiru/v2/x/common/testutil/testapp" | |
| "github.com/NibiruChain/nibiru/v2/x/evm" | |
| "github.com/NibiruChain/nibiru/v2/x/evm/embeds" | |
| "github.com/NibiruChain/nibiru/v2/x/evm/evmtest" | |
| "github.com/NibiruChain/nibiru/v2/x/evm/statedb" | |
| ) |
| func debugDirtiesCountMismatch(db *statedb.StateDB, t *testing.T) string { | ||
| lines := []string{} | ||
| dirties := db.Dirties() | ||
| stateObjects := db.StateObjects() | ||
| for addr, dirtyCountForAddr := range dirties { | ||
| lines = append(lines, fmt.Sprintf("Dirty addr: %s, dirtyCountForAddr=%d", addr, dirtyCountForAddr)) | ||
|
|
||
| // Inspect the actual state object | ||
| maybeObj := stateObjects[addr] | ||
| if maybeObj == nil { | ||
| lines = append(lines, " no state object found!") | ||
| continue | ||
| } | ||
| obj := *maybeObj | ||
|
|
||
| lines = append(lines, fmt.Sprintf(" balance: %s", obj.Balance())) | ||
| lines = append(lines, fmt.Sprintf(" suicided: %v", obj.Suicided)) | ||
| lines = append(lines, fmt.Sprintf(" dirtyCode: %v", obj.DirtyCode)) | ||
|
|
||
| // Print storage state | ||
| lines = append(lines, fmt.Sprintf(" len(obj.DirtyStorage) entries: %d", len(obj.DirtyStorage))) | ||
| for k, v := range obj.DirtyStorage { | ||
| lines = append(lines, fmt.Sprintf(" key: %s, value: %s", k.Hex(), v.Hex())) | ||
| origVal := obj.OriginStorage[k] | ||
| lines = append(lines, fmt.Sprintf(" origin value: %s", origVal.Hex())) | ||
| } | ||
| } | ||
|
|
||
| t.Log("debugDirtiesCountMismatch:\n", strings.Join(lines, "\n")) | ||
| return "" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring debugDirtiesCountMismatch function signature.
The function currently returns an empty string which serves no purpose. Consider either:
- Making it return void (no return value)
- Returning the generated debug string instead of logging it directly
Option 1 (preferred) - Return void:
-func debugDirtiesCountMismatch(db *statedb.StateDB, t *testing.T) string {
+func debugDirtiesCountMismatch(db *statedb.StateDB, t *testing.T) {
// ... existing code ...
t.Log("debugDirtiesCountMismatch:\n", strings.Join(lines, "\n"))
- return ""
}Option 2 - Return the debug string:
func debugDirtiesCountMismatch(db *statedb.StateDB, t *testing.T) string {
// ... existing code ...
- t.Log("debugDirtiesCountMismatch:\n", strings.Join(lines, "\n"))
- return ""
+ return fmt.Sprintf("debugDirtiesCountMismatch:\n%s", strings.Join(lines, "\n"))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func debugDirtiesCountMismatch(db *statedb.StateDB, t *testing.T) string { | |
| lines := []string{} | |
| dirties := db.Dirties() | |
| stateObjects := db.StateObjects() | |
| for addr, dirtyCountForAddr := range dirties { | |
| lines = append(lines, fmt.Sprintf("Dirty addr: %s, dirtyCountForAddr=%d", addr, dirtyCountForAddr)) | |
| // Inspect the actual state object | |
| maybeObj := stateObjects[addr] | |
| if maybeObj == nil { | |
| lines = append(lines, " no state object found!") | |
| continue | |
| } | |
| obj := *maybeObj | |
| lines = append(lines, fmt.Sprintf(" balance: %s", obj.Balance())) | |
| lines = append(lines, fmt.Sprintf(" suicided: %v", obj.Suicided)) | |
| lines = append(lines, fmt.Sprintf(" dirtyCode: %v", obj.DirtyCode)) | |
| // Print storage state | |
| lines = append(lines, fmt.Sprintf(" len(obj.DirtyStorage) entries: %d", len(obj.DirtyStorage))) | |
| for k, v := range obj.DirtyStorage { | |
| lines = append(lines, fmt.Sprintf(" key: %s, value: %s", k.Hex(), v.Hex())) | |
| origVal := obj.OriginStorage[k] | |
| lines = append(lines, fmt.Sprintf(" origin value: %s", origVal.Hex())) | |
| } | |
| } | |
| t.Log("debugDirtiesCountMismatch:\n", strings.Join(lines, "\n")) | |
| return "" | |
| } | |
| func debugDirtiesCountMismatch(db *statedb.StateDB, t *testing.T) { | |
| lines := []string{} | |
| dirties := db.Dirties() | |
| stateObjects := db.StateObjects() | |
| for addr, dirtyCountForAddr := range dirties { | |
| lines = append(lines, fmt.Sprintf("Dirty addr: %s, dirtyCountForAddr=%d", addr, dirtyCountForAddr)) | |
| // Inspect the actual state object | |
| maybeObj := stateObjects[addr] | |
| if maybeObj == nil { | |
| lines = append(lines, " no state object found!") | |
| continue | |
| } | |
| obj := *maybeObj | |
| lines = append(lines, fmt.Sprintf(" balance: %s", obj.Balance())) | |
| lines = append(lines, fmt.Sprintf(" suicided: %v", obj.Suicided)) | |
| lines = append(lines, fmt.Sprintf(" dirtyCode: %v", obj.DirtyCode)) | |
| // Print storage state | |
| lines = append(lines, fmt.Sprintf(" len(obj.DirtyStorage) entries: %d", len(obj.DirtyStorage))) | |
| for k, v := range obj.DirtyStorage { | |
| lines = append(lines, fmt.Sprintf(" key: %s, value: %s", k.Hex(), v.Hex())) | |
| origVal := obj.OriginStorage[k] | |
| lines = append(lines, fmt.Sprintf(" origin value: %s", origVal.Hex())) | |
| } | |
| } | |
| t.Log("debugDirtiesCountMismatch:\n", strings.Join(lines, "\n")) | |
| } |
| @@ -272,7 +272,7 @@ func (k *Keeper) ApplyEvmMsg(ctx sdk.Context, | |||
| intrinsicGas, err := k.GetEthIntrinsicGas(ctx, msg, evmConfig.ChainConfig, contractCreation) | |||
| if err != nil { | |||
| // should have already been checked on Ante Handler | |||
| return nil, errors.Wrap(err, "intrinsic gas failed") | |||
| return nil, evmObj, errors.Wrap(err, "intrinsic gas failed") | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing error handling paths.
The current implementation initializes the EVM object before certain validations (like intrinsic gas checks). If these validations fail, creating the EVM object was unnecessary.
Consider restructuring the code to initialize the EVM object after basic validations:
func (k *Keeper) ApplyEvmMsg(...) (resp *evm.MsgEthereumTxResponse, evmObj *vm.EVM, err error) {
var (
ret []byte
vmErr error
)
stateDB := statedb.New(ctx, k, txConfig)
- evmObj = k.NewEVM(ctx, msg, evmConfig, tracer, stateDB)
intrinsicGas, err := k.GetEthIntrinsicGas(ctx, msg, evmConfig.ChainConfig, contractCreation)
if err != nil {
- return nil, evmObj, errors.Wrap(err, "intrinsic gas failed")
+ return nil, nil, errors.Wrap(err, "intrinsic gas failed")
}
if leftoverGas < intrinsicGas {
- return nil, evmObj, errors.Wrapf(...)
+ return nil, nil, errors.Wrapf(...)
}
+ evmObj = k.NewEVM(ctx, msg, evmConfig, tracer, stateDB)Also applies to: 286-286, 306-306, 336-336, 355-355, 364-364, 368-368, 384-384
Purpose / Abstract
Fixing Dirty State Application
The
statedb.StateDBstruct enforces ether balance changes with journal entries using a setter. In order to maintain consistency between theStateDBand the broader multistore when precompiled contracts transfer ether, theStateDBneeds to be manually modified.This was implemented inside of "precompile/funtoken.go", where I wrapped both calls on the Bank keeper with a safer function that adds a balance change
statedb.JournalChangeto the db in the event that the asset minted, burned, or sent is a NIBI (ether).Reverts
Since a revert rolls back changes and fails the transaction, this operation is generally safe. The transaction handler in the Cosmos-SDK already branches a cached context off at the beginning of the transaction lifecycle, and that outermost context is only written if the entire transaction is going to succeed. That's why panics and other errors within gRPC TxMsgs don't require any direct cleanup. If reverting fails the transaction, it means the precompile changes will get reverted as well.
The problem is that Revert in the EVM can be caught with try/catch for external contract calls.
Dynamic gas to handle large contract input
There was already a function for this used in the Wasm precompile. It's been added to the FunToken one for consistency.
Completion Checklist