fix(evm): Guarantee that gas consumed during any send operation of the "NibiruBankKeeper" depends only on the "bankkeeper.BaseKeeper"'s gas consumption. #2119
Conversation
…ndent on StateDB existence wip!
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (14)
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit can generate a title for your PR based on the changes.Add @coderabbitai placeholder anywhere in the title of your PR and CodeRabbit will replace it with a title based on the changes in the PR. You can change the placeholder by changing the 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
x/evm/keeper/bank_extension.go (1)
43-46: Redundant Ether balance check inMintCoinsThe function
findEtherBalanceChangeFromCoins(coins)is called to determine if an Ether balance change occurs (line 43). However, sinceMintCoinsalways increases the coin supply, this check may be unnecessary.Consider removing the conditional check if
MintCoinsalways affects the Ether balance, simplifying the code.CHANGELOG.md (2)
45-47: Improve changelog entry formatting and clarity.The entry should be on a single line for better readability. Consider this format:
-- [#2xxx](https://github.com/NibiruChain/nibiru/pull/2xxx) - fix(evm): Guarantee -that gas consumed during any send operation of the "NibiruBankKeeper" depends -only on the "bankkeeper.BaseKeeper"'s gas consumption. +[#2119](https://github.com/NibiruChain/nibiru/pull/2119) - fix(evm): Guarantee that gas consumed during NibiruBankKeeper send operations depends only on bankkeeper.BaseKeeper's gas consumption
45-47: Enhance changelog entry with more context.Consider adding more details about the improvements:
-- [#2xxx](https://github.com/NibiruChain/nibiru/pull/2xxx) - fix(evm): Guarantee -that gas consumed during any send operation of the "NibiruBankKeeper" depends -only on the "bankkeeper.BaseKeeper"'s gas consumption. +[#2119](https://github.com/NibiruChain/nibiru/pull/2119) - fix(evm): Guarantee consistent gas consumption in NibiruBankKeeper send operations by relying solely on bankkeeper.BaseKeeper, preventing EVM StateDB influence on gas calculationsThis provides better context about the problem being solved and its impact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)go.mod(1 hunks)x/evm/keeper/bank_extension.go(5 hunks)x/evm/keeper/bank_extension_test.go(1 hunks)x/evm/keeper/msg_server.go(1 hunks)x/evm/keeper/statedb.go(1 hunks)
🔇 Additional comments (2)
x/evm/keeper/bank_extension.go (1)
128-132: Ensure StateDB synchronization after coin transfers
In SendCoins, when Ether balances change, SyncStateDBWithAccount is called for both fromAddr and toAddr (lines 129-130). Ensure that this synchronization occurs in all cases where Ether balances are affected to maintain consistency between the bank and the EVM StateDB.
go.mod (1)
69-69: Dependency update: Addition of zerolog
The addition of github.com/rs/zerolog v1.32.0 is appropriate if logging enhancements are required.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2119 +/- ##
==========================================
+ Coverage 64.67% 64.80% +0.12%
==========================================
Files 273 273
Lines 21601 21638 +37
==========================================
+ Hits 13970 14022 +52
+ Misses 6655 6644 -11
+ Partials 976 972 -4
|
Purpose / Abstract
The prior conditional behavior in the
NibiruBankKeeperwhere we switched out the gas config to try to prevent the EVMStateDBfrom affecting gas consumption during send operations was not effective.I set up some test cases where I'd bank send in the same manner when the
StateDBwas nil and when theStateDBwas defined to compare the gas results:Code Changes
bankkeeper.BaseKeeper, making the value consistent regardless of whether theEVM
StateDBexistsprevent regressions. link to file
deferstatement in evm/keeper/msg_server.go to only set the valuefor the
NibiruBankKeeperStateDBpointer ifcommitis true and the statetransition is a success