chore(evm): adjust extra decimals on stateDB methods #2897
Conversation
WalkthroughThe pull request introduces significant updates to the Evmos project, including enhancements to the state object management, balance adjustments, and improved decimal handling in wrapper functions. Key changes involve the addition of methods for balance manipulation, modifications to existing methods for better tracking of state changes, and updates to function visibility. The changelog reflects ongoing efforts to improve functionality, compatibility, and performance across the ecosystem. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
x/evm/wrappers/bank.go (1)
102-102: Approve change and summarize overall impactThe change from
convertCoinsFrom18DecimalstoConvertCoinsFrom18Decimalsin theSendCoinsFromModuleToAccountmethod is consistent with all previous changes in this file.These changes collectively represent a refactoring to make all conversion functions public (exported). While the functionality remains the same, the increased visibility of these functions may have implications for the broader codebase:
- It allows for more flexible usage of these functions in other packages.
- It potentially exposes these functions as part of the public API, which may have maintenance and backwards compatibility considerations.
Ensure that this change aligns with the overall architecture and design principles of the project.
Consider documenting these newly exported functions and updating any relevant API documentation to reflect these changes.
x/evm/statedb/state_object.go (5)
Line range hint
104-108: LGTM: Adjustment for extra decimals in AddBalance.The addition of
wrappers.AdjustExtraDecimalsBigInt(amount)aligns with the PR objective of handling extra decimals in stateDB methods. This change ensures that the balance is adjusted correctly before being added.Consider adding a comment explaining the purpose of the
AdjustExtraDecimalsBigIntcall for better code readability:// Adjust the amount for extra decimals before adding to balance amount = wrappers.AdjustExtraDecimalsBigInt(amount)
Line range hint
114-118: LGTM: Adjustment for extra decimals in SubBalance.The addition of
wrappers.AdjustExtraDecimalsBigInt(amount)is consistent with the changes inAddBalanceand aligns with the PR objective. This ensures that the balance is adjusted correctly before being subtracted.Similar to the suggestion for
AddBalance, consider adding a comment for clarity:// Adjust the amount for extra decimals before subtracting from balance amount = wrappers.AdjustExtraDecimalsBigInt(amount)
Line range hint
122-126: LGTM: Added journal entry for balance changes in SetBalance.The addition of a journal entry for balance changes improves the tracking and potential reverting of state changes. This enhancement aligns with best practices for maintaining state in blockchain systems.
Consider adding a comment explaining the purpose of the journal entry:
// Append a journal entry to track the balance change s.db.journal.append(balanceChange{ account: &s.address, prev: new(big.Int).Set(s.account.Balance), })
Line range hint
183-188: LGTM: Added journal entry for code changes in SetCode.The addition of a journal entry for code changes improves the tracking and potential reverting of contract code modifications. This enhancement aligns with best practices for maintaining state in blockchain systems.
Consider adding a comment explaining the purpose of the journal entry:
// Append a journal entry to track the code change s.db.journal.append(codeChange{ account: &s.address, prevhash: s.CodeHash(), prevcode: prevcode, })
Line range hint
1-270: Overall assessment: Changes are well-implemented and align with PR objectives.The modifications to
AddBalance,SubBalance,SetBalance, andSetCodemethods improve the handling of extra decimals and enhance state tracking through journal entries. These changes are consistent with the PR objectives and follow good practices for blockchain state management.Consider documenting the rationale behind the extra decimal adjustments in the package or file-level comments. This would help future developers understand the purpose and importance of these modifications in the context of the EVM state management.
x/evm/wrappers/utils_test.go (1)
210-263: LGTM: Comprehensive test function for AdjustExtraDecimalsBigInt.The new
TestZeroExtraDecimalsBigIntfunction is well-structured and covers a good range of scenarios, including edge cases. The use of table-driven tests and testing both 6 and 18 decimal configurations is commendable.Consider adding a test case for a very large number (e.g., close to
math.MaxInt64) to ensure the function handles large inputs correctly.CHANGELOG.md (1)
67-67: Consider wrapping this line to improve readability.The line exceeds the recommended length of 120 characters. Consider wrapping it to improve readability and consistency with markdown best practices.
Here's a suggested way to wrap the line:
- (evm) [#2897](https://github.com/evmos/evmos/pull/2897) Adjust extra decimals in `AddBalance` and `SubBalance` methods on `stateObject` in state DB.🧰 Tools
🪛 Markdownlint
67-67: Expected: 120; Actual: 150
Line length(MD013, line-length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- x/evm/statedb/state_object.go (3 hunks)
- x/evm/wrappers/bank.go (4 hunks)
- x/evm/wrappers/utils.go (4 hunks)
- x/evm/wrappers/utils_test.go (4 hunks)
🧰 Additional context used
🪛 Markdownlint
CHANGELOG.md
67-67: Expected: 120; Actual: 150
Line length(MD013, line-length)
🔇 Additional comments (15)
x/evm/wrappers/utils.go (4)
Line range hint
18-28: LGTM: Function renamed to follow Go conventionsThe function has been correctly renamed from
mustConvertEvmCoinTo18DecimalstoMustConvertEvmCoinTo18Decimals, following Go's naming convention for exported functions. The logic remains unchanged and appropriate for its purpose.
Line range hint
31-41: LGTM: Function renamed to follow Go conventionsThe function has been correctly renamed from
convertEvmCoinFrom18DecimalstoConvertEvmCoinFrom18Decimals, following Go's naming convention for exported functions. The logic and error handling remain appropriate and unchanged.
Line range hint
44-59: LGTM: Function renamed to follow Go conventionsThe function has been correctly renamed from
convertCoinsFrom18DecimalstoConvertCoinsFrom18Decimals, following Go's naming convention for exported functions. The logic remains unchanged and correctly handles the conversion of multiple coins, only modifying the EVM coin while leaving others untouched.
61-73: Verify the behavior of the new AdjustExtraDecimalsBigInt functionThe new function
AdjustExtraDecimalsBigInthas been added to handle cases where the decimal configuration differs from 18 decimals. While the implementation looks correct, please consider the following points:
- Verify that this function behaves correctly for all possible decimal configurations.
- There might be a potential precision loss when scaling down and then up again. Consider if this is acceptable for your use case or if additional measures need to be taken to minimize precision loss.
To verify the function's behavior, please run the following test cases:
func TestAdjustExtraDecimalsBigInt(t *testing.T) { testCases := []struct { name string input *big.Int decimals int64 expected *big.Int }{ {"18 decimals", big.NewInt(1e18), 18, big.NewInt(1e18)}, {"6 decimals", big.NewInt(1e18), 6, big.NewInt(1e6)}, {"zero amount", big.NewInt(0), 6, big.NewInt(0)}, {"large number", new(big.Int).Exp(big.NewInt(10), big.NewInt(30), nil), 6, new(big.Int).Exp(big.NewInt(10), big.NewInt(18), nil)}, } for _, 8000 tc := range testCases { t.Run(tc.name, func(t *testing.T) { // Set the EVM coin decimals for the test config.SetEVMCoinDecimals(sdk.NewInt(tc.decimals)) result := AdjustExtraDecimalsBigInt(tc.input) if result.Cmp(tc.expected) != 0 { t.Errorf("Expected %s, got %s", tc.expected.String(), result.String()) } }) } }This test will help ensure the function behaves correctly for various inputs and decimal configurations.
x/evm/wrappers/bank.go (4)
63-63: Approve consistent changeThe change from
convertEvmCoinFrom18DecimalstoConvertEvmCoinFrom18Decimalsin theBurnAmountFromAccountmethod is consistent with the previous change inMintAmountToAccount. This maintains the pattern of making the conversion functions public.
86-86: Approve change and note "Must" prefix implicationThe change from
mustConvertEvmCoinTo18DecimalstoMustConvertEvmCoinTo18Decimalsis consistent with the pattern of making conversion functions public.Note that the "Must" prefix in the function name implies that this function will panic if it fails. This is a common Go pattern for functions that shouldn't fail under normal circumstances. Ensure that the function's implementation matches this expectation.
93-93: Approve consistent changeThe change from
convertCoinsFrom18DecimalstoConvertCoinsFrom18Decimalsin theSendCoinsFromAccountToModulemethod is consistent with the previous changes. This maintains the pattern of making the conversion functions public.
45-45: Approve change and verify impact on codebaseThe change from
convertEvmCoinFrom18DecimalstoConvertEvmCoinFrom18Decimalsis part of making the conversion functions public. While this change is correct, it's important to ensure that all other parts of the codebase using this function have been updated accordingly.To verify the impact and consistency of this change across the codebase, run the following script:
✅ Verification successful
Verified function name update and consistent usage
All instances of
convertEvmCoinFrom18Decimalshave been replaced withConvertEvmCoinFrom18Decimalsacross the codebase, ensuring consistency and proper function visibility.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of the old function name and verify the new function name is used consistently. # Test 1: Search for any remaining usage of the old function name echo "Searching for any remaining usage of convertEvmCoinFrom18Decimals:" rg --type go 'convertEvmCoinFrom18Decimals' # Test 2: Verify the new function name is used consistently echo "Verifying usage of ConvertEvmCoinFrom18Decimals:" rg --type go 'ConvertEvmCoinFrom18Decimals'Length of output: 883
x/evm/statedb/state_object.go (1)
14-14: LGTM: New import for wrappers package.The addition of the
wrapperspackage import is necessary for the new functionality inAddBalanceandSubBalancemethods.x/evm/wrappers/utils_test.go (5)
4-4: LGTM: Package name change aligns with Go testing conventions.The change from
package wrapperstopackage wrappers_testis appropriate. This allows the tests to import and use thewrapperspackage as an external package, which is a good practice for testing exported functionality.
7-8: LGTM: Import changes are consistent with new test function and package structure.The addition of the
fmtimport and thewrapperspackage import are appropriate given the package name change and the new test function.Also applies to: 15-15
82-82: LGTM: Function call updated to use wrappers package.The change to
wrappers.MustConvertEvmCoinTo18Decimals(tc.coin)is consistent with the package restructuring and allows proper testing of the exported functionality.
146-146: LGTM: Function call updated to use wrappers package.The change to
wrappers.ConvertEvmCoinFrom18Decimals(tc.coin)is consistent with the package restructuring and allows proper testing of the exported functionality.
Line range hint
1-263: Summary: Improved test structure and coverage.The changes in this file successfully:
- Align the package structure with Go testing conventions.
- Update existing tests to work with the new package structure.
- Add comprehensive testing for the
AdjustExtraDecimalsBigIntfunction.These improvements enhance the overall test coverage and maintainability of the codebase. Great job!
CHANGELOG.md (1)
Line range hint
1-66: LGTM: Changelog entries are well-documented and follow a consistent format.The changes in the "Unreleased" section of the CHANGELOG.md file are well-documented and follow a consistent format. The entries provide clear information about improvements and bug fixes, which is helpful for users and developers to understand the changes in the upcoming release.
🧰 Tools
🪛 Markdownlint
64-64: Expected: 120; Actual: 139
Line length(MD013, line-length)
67-67: Expected: 120; Actual: 150
Line length(MD013, line-length)
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
Reviewers Checklist
All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.
I have...
Unreleasedsection inCHANGELOG.mdSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor