feat(evm-funtoken-precompile): Implement "balance", "bankBalance", and "whoAmI" methods #2107
Conversation
WalkthroughThe pull request includes updates to the Nibiru EVM, enhancing its functionality and consistency. Key changes involve the addition of new features, bug fixes, and improvements, particularly in the handling of ERC20 tokens and FunTokens. Notable modifications include the introduction of new methods in the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Tool Failures:Tool Failure Count: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 #2107 +/- ##
==========================================
- Coverage 64.75% 64.66% -0.09%
==========================================
Files 273 273
Lines 21414 21598 +184
==========================================
+ Hits 13866 13966 +100
- Misses 6589 6655 +66
- Partials 959 977 +18
|
Unique-Divine
changed the title
Unique-Divine
force-pushed
the
ud/funtoken1-bank-send
branch
from
fd90020 to
06f46f3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (36)
x/evm/embeds/contracts/TestERC20TransferThenPrecompileSend.sol (1)
Line range hint
37-42: Consider improving error message formattingThe error message correctly reflects the renamed method. However, the message readability could be enhanced.
Consider adding spaces between the concatenated parts:
- "IFunToken.sendToBank succeeded but transferred the wrong amount", - "sentAmount ", + "IFunToken.sendToBank succeeded but transferred the wrong amount. ", + "Sent amount: ", Strings.toString(sentAmount), - "expected ", + ", Expected: ", Strings.toString(precompileAmount)x/evm/embeds/contracts/TestPrecompileSelfCallRevert.sol (1)
Line range hint
52-56: Consider improving error message formatting for better readability.While the error message is descriptive, its readability could be enhanced.
Consider this format:
- "IFunToken.sendToBank succeeded but transferred the wrong amount", - "sentAmount ", - Strings.toString(nativeAmount), - "expected ", - Strings.toString(precompileAmount) + "IFunToken.sendToBank succeeded but transferred wrong amount. ", + "Sent: ", + Strings.toString(nativeAmount), + " Expected: ", + Strings.toString(precompileAmount)x/evm/embeds/contracts/TestFunTokenPrecompileLocalGas.sol (2)
Line range hint
20-34: Consider optimizing error message constructionWhile the implementation is correct, the error message construction using multiple string concatenations could be gas-intensive. Consider using a more efficient approach.
- string.concat( - "IFunToken.sendToBank succeeded but transferred the wrong amount", - "sentAmount ", - Strings.toString(sentAmount), - "expected ", - Strings.toString(amount) - ) + string(abi.encodePacked( + "IFunToken.sendToBank succeeded but transferred wrong amount: sent ", + Strings.toString(sentAmount), + ", expected ", + Strings.toString(amount) + ))
Line range hint
44-58: Consider extracting common validation logicBoth functions share identical validation logic. Consider extracting this into a private helper function to follow the DRY principle.
+ function validateAmount(uint256 sentAmount, uint256 expectedAmount) private pure { + require( + sentAmount == expectedAmount, + string(abi.encodePacked( + "IFunToken.sendToBank succeeded but transferred wrong amount: sent ", + Strings.toString(sentAmount), + ", expected ", + Strings.toString(expectedAmount) + )) + ); + } + function callBankSend(uint256 amount, string memory bech32Recipient) public { uint256 sentAmount = FUNTOKEN_PRECOMPILE.sendToBank( erc20, amount, bech32Recipient ); - require( - sentAmount == amount, - string.concat(...) - ); + validateAmount(sentAmount, amount); }x/evm/embeds/contracts/TestNativeSendThenPrecompileSend.sol (1)
Line range hint
40-57: Add documentation and input validation.While the implementation is correct, consider these improvements:
- Add NatSpec documentation to explain the function's purpose, parameters, and return values
- Add input validation for
precompileAmount(e.g., check for zero amount)Here's a suggested improvement:
+ /// @notice Sends tokens directly to a bank address using the FunToken precompile + /// @param precompileRecipient The recipient address in bank format + /// @param precompileAmount The amount of tokens to send + /// @dev This is a simplified version of nativeSendThenPrecompileSend that only handles precompile operations function justPrecompileSend( string memory precompileRecipient, uint256 precompileAmount ) public { + require(precompileAmount > 0, "Amount must be greater than zero"); + require(bytes(precompileRecipient).length > 0, "Recipient address cannot be empty"); uint256 sentAmount = FUNTOKEN_PRECOMPILE.sendToBank(x/evm/precompile/errors.go (1)
60-64: Consider adding nil check for args parameter.While the implementation is correct, it would be more robust to add validation for the
argsparameter being nil.func assertNumArgs(args []any, wantArgsLen int) error { + if args == nil { + return fmt.Errorf("expected %d arguments but got nil args", wantArgsLen) + } lenArgs := len(args) if lenArgs != wantArgsLen { return fmt.Errorf("expected %d arguments but got %d; args: %v", wantArgsLen, lenArgs, args) } return nil }x/evm/precompile/oracle.go (1)
104-104: Consider enhancing error messages for argument validation.While the argument validation is correct, consider providing more specific error messages to help developers debug issues more easily.
- if e := assertNumArgs(args, 1); e != nil { + if e := assertNumArgs(args, 1, "queryExchangeRate"); e != nil {x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json (2)
139-168: Consider improvements to thewhoAmIfunction.While the function successfully implements the address format conversion requirement, consider these improvements:
- Change state mutability to
viewsince this is a read-only operation- Consider a more descriptive name like
getAddressFormatsorconvertAddressto better reflect its purposeThe current implementation correctly handles both hex and bech32 address formats as required by issue #2049.
Line range hint
6-168: Well-structured precompile interface design.The overall interface design effectively bridges EVM and Cosmos functionality, providing a clean API for users to interact with both ERC20 and bank features. The consistent return of address formats across functions and the separation of concerns between balance queries shows good architectural thinking.
Consider documenting these precompile methods in the README or developer documentation to help users understand the new simplified workflow compared to the previous gRPC approach.
x/evm/precompile/wasm_parse.go (3)
Line range hint
70-126: Consider standardizing error handling.While the function is well-structured, consider wrapping all validation errors consistently with additional context, similar to how it's done in other functions.
For example:
if !ok { - err = ErrArgTypeValidation("string admin", args[argIdx]) + err = fmt.Errorf("%w: failed to parse admin", + ErrArgTypeValidation("string admin", args[argIdx])) return }
Line range hint
177-208: Consider consistent variable naming across functions.The function uses
argsIdxwhile other functions useargIdx. Consider standardizing the naming convention.- argsIdx := 0 + argIdx := 0
Line range hint
209-241: Fix incorrect error message in type validation.The error message refers to "BankCoin[] funds" but should describe the execute messages structure instead.
- err = ErrArgTypeValidation("BankCoin[] funds", arg) + err = ErrArgTypeValidation("WasmExecuteMsg[] execMsgs", arg)eth/rpc/backend/account_info.go (2)
64-65: Enhance comment clarity for height validation.While the comment formatting improves readability, consider expanding it to better explain the EVM context:
-// if the height is equal to zero, meaning the query condition of the block -// is either "pending" or "latest" +// If the height is equal to zero, it indicates that the query condition +// specified "pending" (next block) or "latest" (most recent block) +// in the RPC request, requiring us to fetch the current block number.
Line range hint
1-277: Consider architectural changes for bank balance support.Given that the PR objectives include implementing both EVM and bank balance queries, consider whether the
GetBalancemethod should be extended or a new method should be added to support querying bank module balances. This would align with the goal of providing a unified interface for both EVM and Cosmos-SDK bank balances.Some considerations:
- The current implementation only handles EVM balances
- Bank module balances use different denomination handling
- The PR aims to simplify access to both balance types
Would you like assistance in designing an interface that supports both balance types while maintaining clean separation of concerns?
x/evm/precompile/precompile.go (1)
225-228: Architecture Review: Clean integration with existing precompile infrastructure.The new FunToken methods seamlessly integrate with the existing precompile infrastructure:
- They inherit the robust gas calculation system
- They benefit from the state isolation provided by
OnRunStart- They follow the established pattern for mutation vs. query operations
This implementation aligns well with the PR objective of simplifying access to bank and ERC20 balances while maintaining the system's integrity.
x/evm/keeper/funtoken_from_erc20_test.go (1)
379-380: LGTM! Consider documenting gas limit rationale.The method rename and gas limit changes are consistent with other test cases. Since this test specifically deals with malicious contracts and gas limits, it might be helpful to add a comment explaining why this specific gas limit value was chosen.
Consider adding a comment explaining the gas limit choice:
true, + // Using standard gas limit as the malicious transfer function should fail even with normal gas limits evmtest.FunTokenGasLimitSendToEvm, "sendToBank",x/evm/precompile/wasm.go (3)
67-67: Use Go-style comments for method references.Replace square brackets with backticks for method references to follow Go's documentation conventions.
-// "[decomposeInput]" parses methods directly from the ABI. +// `decomposeInput` parses methods directly from the ABI.
76-78: Clarify gas meter initialization.The comment explains that gas consumption is guaranteed to be safe but doesn't specify where/how the gas meter is initialized. Consider adding this context to make the safety guarantee more clear.
287-287: Consider enhancing error handling for multiple contract calls.While the rename to
parseArgsWasmExecuteMultiis good, consider adding transaction rollback handling in case one of the multiple contract calls fails.x/evm/precompile/funtoken_test.go (2)
92-128: Consider adding more edge cases to TestWhoAmI.While the test covers basic functionality well, consider adding these edge cases:
- Empty string address
- Malformed Bech32 address
- Malformed hex address
- Case sensitivity in hex addresses
Line range hint
379-423: Consider using a constant for the insufficient gas value.Replace the magic number
50_000with a named constant to improve code maintainability and clarity.+ const insufficientGasLimit = 50_000 - big.NewInt(50_000), // customGas - too small + big.NewInt(insufficientGasLimit), // customGas - too smallx/evm/embeds/artifacts/contracts/TestNativeSendThenPrecompileSend.sol/TestNativeSendThenPrecompileSend.json (1)
Line range hint
1-65: Contract changes support PR objectives.The test contract has been enhanced to provide better coverage of the precompile functionality, which aligns with the PR's goal of simplifying user interactions with the Nibiru blockchain. The separation of concerns between native and precompile sends through the new
justPrecompileSendfunction improves testability.Consider adding similar test contracts for the new balance query methods (
balance,bankBalance,whoAmI) to maintain consistent test coverage across all precompile features.eth/rpc/rpcapi/eth_api_test.go (1)
373-376: Consider handling errors from WaitForNextBlock.While waiting for 5 blocks is a good approach to ensure transaction finality, the error handling could be improved for better test debugging.
Consider applying this change:
- for i := 0; i < 5; i++ { - _ = s.network.WaitForNextBlock() - } + for i := 0; i < 5; i++ { + if err := s.network.WaitForNextBlock(); err != nil { + s.T().Fatalf("failed to wait for block %d: %v", i+1, err) + } + }CHANGELOG.md (1)
103-104: Improve changelog entry for PR #2107.The current entry is too terse and lacks important details about the implemented methods. Consider expanding it to:
-- [#2107](https://github.com/NibiruChain/nibiru/pull/2107) - -feat(evm-funtoken-precompile): Implement methods: balance, bankBalance, whoAmI +[#2107](https://github.com/NibiruChain/nibiru/pull/2107) - feat(evm-funtoken-precompile): Implement query methods for retrieving bank and ERC20 balances from EVM transactions. The new methods include: +- `balance`: Query ERC20 token balance +- `bankBalance`: Query bank module balance +- `whoAmI`: Query address information + +This change simplifies balance queries by removing the need to use the gRPC query server.x/evm/embeds/contracts/IFunToken.sol (4)
7-9: Improve documentation clarity in interface descriptionThe current documentation contains redundant phrases and could be clarified for better understanding.
Suggestion:
"Implements functionality for sending ERC20 tokens and bank coins to Nibiru accounts using the Nibiru Bech32 address and the 'FunToken' mapping between ERC20 tokens and bank denominations."
33-43: Addviewmodifier to thebalancefunctionThe
balancefunction appears to be read-only and does not modify state. Adding theviewmodifier can optimize gas usage and clearly communicate the function's intent.Apply this diff to add the
viewmodifier:function balance( address who, address funtoken ) - external + external view returns ( uint256 erc20Balance, uint256 bankBalance, FunToken memory token, NibiruAccount memory whoAddrs );
45-48: Addviewmodifier to thebankBalancefunctionSince the
bankBalancefunction likely does not modify state, adding theviewmodifier optimizes gas usage and denotes its read-only nature.Apply this diff to add the
viewmodifier:function bankBalance( address who, string calldata bankDenom ) - external + external view returns (uint256 bankBalance, NibiruAccount memory whoAddrs);
50-52: Addviewmodifier to thewhoAmIfunctionThe
whoAmIfunction seems to be a read-only function that does not modify state. Adding theviewmodifier indicates its read-only status and can improve gas efficiency.Apply this diff to add the
viewmodifier:function whoAmI( string calldata who ) - external + external view returns (NibiruAccount memory whoAddrs);x/evm/precompile/funtoken.go (3)
207-209: Implement event emission for balance changesThe
sendToBankfunction contains TODO comments for emitting EVM events related to the balance changes of the sender and recipient. Implementing these events is crucial for enhancing traceability and allowing clients to react to balance updates within the EVM.
213-247: Refactor argument parsing to reduce code duplicationThe argument parsing functions (
parseArgsSendToBank,parseArgsBalance,parseArgsBankBalance, andparseArgsWhoAmI) contain similar logic for validating argument counts and types. Consider extracting common validation logic into a shared utility function to improve maintainability and reduce code duplication.Also applies to: 311-352, 393-431, 472-502
285-289: Handle potential errors when retrieving bank balanceIn the
balancefunction, when retrieving the bank balance usingGetBalance, consider handling any potential errors that might occur. WhileGetBalanceusually returns a zero balance for non-existent accounts, explicitly checking for errors can improve code robustness.x/evm/keeper/msg_server.go (5)
358-358: Address the TODO comment: Clarify the inline commentsThe inline comment on line 358 contains a TODO with "UD-DEBUG: Clarify text below." This suggests that the explanation below might not be clear or complete. It's important to resolve TODO comments before merging to maintain code clarity and avoid confusion for future maintainers.
Consider revising or expanding the comment to clearly explain the gas refund logic.
364-368: Improve the clarity of the gas refund commentsThe comments explaining the gas refund logic (lines 364-368) could be more concise and clearer. The current explanation might be confusing for readers unfamiliar with the context.
Consider rephrasing the comments for better readability:
-// -// EIP-3529: refunds are capped to gasUsed / 5 -// We evaluate "fullRefundLeftoverGas" and use only the gas consumed (not the -// gas limit) if the `ApplyEvmMsg` call originated from a state transition -// where the chain set the gas limit and not an end-user. +// Gas Refund Logic: +// - Per EIP-3529, refunds are capped at gasUsed / 5. +// - If `fullRefundLeftoverGas` is true (internal calls like FunTokens), we refund 100% of the leftover gas. +// - This means we use the actual gas consumed instead of the gas limit set by the user.
358-371: Consistency in comment formattingThere is inconsistent use of comment styles around the gas refund logic. Some comments start with
//, others with a blank line before them.For better readability and consistency, consider formatting comments uniformly.
-// TODO: UD-DEBUG: Clarify text below. - -// -// EIP-3529: refunds are capped to gasUsed / 5 -// We evaluate "fullRefundLeftoverGas" and use only the gas consumed (not the -// gas limit) if the `ApplyEvmMsg` call originated from a state transition -// where the chain set the gas limit and not an end-user. +// TODO: Clarify the gas refund logic. +// +// Gas Refund Logic: +// - Per EIP-3529, refunds are capped at gasUsed / 5. +// - If `fullRefundLeftoverGas` is true, we refund 100% of the leftover gas for internal calls. +// - This ensures internal transactions are not unfairly charged.
Line range hint
358-384: Consider extracting gas refund logic into a separate functionThe gas refund calculation spans multiple lines and involves conditional logic.
For better modularity and readability, consider extracting this logic into a separate function, e.g.,
calculateGasRefund.func calculateGasRefund(temporaryGasUsed uint64, stateDB *StateDB, refundQuotient uint64) (uint64, uint64) { refund := GasToRefund(stateDB.GetRefund(), temporaryGasUsed, refundQuotient) leftoverGas := temporaryGasUsed + refund gasUsed := temporaryGasUsed - refund return leftoverGas, gasUsed }This makes the
ApplyEvmMsgfunction cleaner and the gas refund logic reusable.
365-369: Correct grammatical error in commentsThere's a grammatical error in the comment on line 367:
"... if the
ApplyEvmMsgcall originated from a state transition where the chain set the gas limit and not an end-user."Consider rephrasing for clarity:
"... if the
ApplyEvmMsgcall originated from a state transition where the chain sets the gas limit, not an end-user."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (26)
CHANGELOG.md(1 hunks)eth/rpc/backend/account_info.go(1 hunks)eth/rpc/rpcapi/eth_api_test.go(2 hunks)x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json(3 hunks)x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json(1 hunks)x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json(1 hunks)x/evm/embeds/artifacts/contracts/TestNativeSendThenPrecompileSend.sol/TestNativeSendThenPrecompileSend.json(2 hunks)x/evm/embeds/artifacts/contracts/TestPrecompileSelfCallRevert.sol/TestPrecompileSelfCallRevert.json(1 hunks)x/evm/embeds/contracts/IFunToken.sol(1 hunks)x/evm/embeds/contracts/TestERC20TransferThenPrecompileSend.sol(2 hunks)x/evm/embeds/contracts/TestFunTokenPrecompileLocalGas.sol(2 hunks)x/evm/embeds/contracts/TestNativeSendThenPrecompileSend.sol(1 hunks)x/evm/embeds/contracts/TestPrecompileSelfCallRevert.sol(1 hunks)x/evm/embeds/embeds.go(1 hunks)x/evm/evmtest/erc20.go(1 hunks)x/evm/keeper/funtoken_from_coin_test.go(8 hunks)x/evm/keeper/funtoken_from_erc20_test.go(3 hunks)x/evm/keeper/keeper.go(0 hunks)x/evm/keeper/msg_server.go(2 hunks)x/evm/precompile/errors.go(1 hunks)x/evm/precompile/funtoken.go(6 hunks)x/evm/precompile/funtoken_test.go(7 hunks)x/evm/precompile/oracle.go(2 hunks)x/evm/precompile/precompile.go(1 hunks)x/evm/precompile/wasm.go(7 hunks)x/evm/precompile/wasm_parse.go(6 hunks)
💤 Files with no reviewable changes (1)
- x/evm/keeper/keeper.go
✅ Files skipped from review due to trivial changes (5)
- x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json
- x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json
- x/evm/embeds/artifacts/contracts/TestPrecompileSelfCallRevert.sol/TestPrecompileSelfCallRevert.json
- x/evm/embeds/embeds.go
- x/evm/evmtest/erc20.go
🔇 Additional comments (34)
x/evm/embeds/contracts/TestERC20TransferThenPrecompileSend.sol (2)
Line range hint 28-32: LGTM: Method rename properly implemented
The change from bankSend to sendToBank is correctly implemented while maintaining the same parameter structure and return value handling.
Line range hint 28-42: Verify complete migration to new method name
Let's ensure all instances of the old method name have been updated across the codebase.
✅ Verification successful
Method rename migration is complete
The old method name bankSend only appears in a documentation file (x/devgas/v1/README.md), while sendToBank is consistently used across all Solidity contracts and Go implementation files. The method rename has been properly implemented across the entire codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of the old method name
echo "Checking for any remaining instances of 'bankSend'..."
rg -l "bankSend" --type sol
# Search for all instances of the new method name to verify consistent usage
echo "Verifying usage of new method name 'sendToBank'..."
rg -l "sendToBank" --type sol
Length of output: 353
Script:
#!/bin/bash
# Search for any remaining instances of the old method name without file type filter
echo "Checking for any remaining instances of 'bankSend'..."
rg "bankSend"
# Search for all instances of the new method name
echo "Verifying usage of new method name 'sendToBank'..."
rg "sendToBank"
# List all Solidity files to verify the extension
echo "Listing Solidity files in the repository..."
fd ".*\.sol$"
Length of output: 6322
x/evm/embeds/contracts/TestFunTokenPrecompileLocalGas.sol (2)
14-19: LGTM: Function signature and documentation are well-defined
The function signature and documentation accurately reflect the renamed method and maintain appropriate parameter types.
37-43: LGTM: Custom gas parameter function is well-documented
The function signature and documentation clearly explain the purpose of the custom gas parameter and maintain appropriate parameter types.
x/evm/embeds/contracts/TestNativeSendThenPrecompileSend.sol (1)
23-38: LGTM! Improved error handling with detailed feedback.
The renaming from bankSend to sendToBank is consistent with the PR objectives, and the enhanced error message now includes both the actual and expected amounts, which will greatly help with debugging.
x/evm/precompile/errors.go (1)
60-64: LGTM! Improved function signature and error reporting.
The change to accept []any instead of just the length is a good improvement as it:
- Prevents potential length calculation errors
- Provides more context in error messages by including the actual arguments
- Makes the function more self-contained
x/evm/precompile/oracle.go (1)
53-65: LGTM! Improved error handling and gas tracking.
The changes enhance the code by:
- Adding explicit error handling
- Properly tracking gas consumption
- Adding clarifying comments about method validation
x/evm/embeds/artifacts/contracts/IFunToken.sol/IFunToken.json (2)
6-68: Well-designed balance query implementation!
The balance function provides a comprehensive solution by returning both ERC20 and bank balances along with token details and address formats. This aligns perfectly with the PR objectives and provides a unified way to query balances without needing separate gRPC calls.
128-128: Good function rename for clarity!
The rename from bankSend to sendToBank improves readability by following a clear verb-object pattern. This makes the function's purpose more immediately apparent.
x/evm/precompile/wasm_parse.go (2)
Line range hint 54-69: LGTM! Function rename improves naming consistency.
The function maintains robust error handling while following the new naming convention.
Line range hint 127-176: LGTM! Well-structured error handling and validation.
The function demonstrates good practices with thorough validation and contextual error messages.
x/evm/keeper/funtoken_from_erc20_test.go (2)
247-248: LGTM! Consistent method rename in error test case.
The changes maintain consistency with the happy path test case while properly testing the error scenario for sending excessive tokens.
223-224: Method rename improves clarity and consistency.
The rename from bankSend to sendToBank better describes the direction of the token transfer. The gas limit constant has been appropriately updated to match.
Let's verify the consistency of this rename across the codebase:
✅ Verification successful
Based on the search results, I can generate the final response:
Method rename is consistently applied across the codebase
The rename from bankSend to sendToBank has been properly implemented across all relevant files. The only remaining instance of bankSend is in documentation (x/devgas/v1/README.md) which appears to be a general example and not related to the actual implementation.
The new name sendToBank is consistently used in:
- Test files (
funtoken_from_erc20_test.go,funtoken_from_coin_test.go) - Core implementation (
funtoken.go,precompile.go) - Smart contracts (
IFunToken.soland test contracts) - Generated artifacts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of 'bankSend' that might have been missed
# and confirm all occurrences of 'sendToBank' are used correctly
# Search for any remaining instances of the old name
echo "Checking for remaining instances of 'bankSend':"
rg "bankSend"
# Verify the new name usage
echo "Verifying 'sendToBank' usage:"
rg "sendToBank"
Length of output: 4701
x/evm/precompile/wasm.go (5)
152-152: LGTM! Improved method naming consistency.
The rename to parseArgsWasmExecute follows a clearer and more consistent naming pattern.
190-190: LGTM! Consistent with the new naming pattern.
The rename to parseArgsWasmQuery aligns with the improved naming convention.
356-356: LGTM! Simplified argument validation.
The change to pass args directly to assertNumArgs suggests a more robust validation in the helper function.
362-362: LGTM! Consistent naming pattern.
The rename to parseArgContractAddr aligns with the new naming convention.
237-237: LGTM! Verify contract instantiation tests.
The rename to parseArgsWasmInstantiate follows the improved naming pattern. Since this is in the critical path for contract creation, ensure integration tests cover this change.
x/evm/precompile/funtoken_test.go (3)
51-69: LGTM: Method renaming is consistent and complete.
The renaming from BankSend to sendToBank has been properly implemented across all test cases, maintaining the original test coverage and validity.
Also applies to: 222-252
130-152: LGTM: Well-structured return type with proper error handling.
The FunTokenWhoAmIReturn type is well-designed with:
- Clear field structure and proper ABI tags
- Comprehensive parsing method
- Consistent error handling
284-344: LGTM: Well-designed return types for balance queries.
The balance return types are well-implemented with:
- Clear separation of concerns
- Proper error handling
- Consistent structure with other return types
x/evm/embeds/artifacts/contracts/TestNativeSendThenPrecompileSend.sol/TestNativeSendThenPrecompileSend.json (2)
64-65: LGTM! Bytecode changes are consistent with contract modifications.
The bytecode updates reflect the addition of justPrecompileSend and the renaming of bankSend to sendToBank.
17-34: LGTM! New function signature aligns with requirements.
The new justPrecompileSend function's parameters correctly support the PR objectives by allowing string addresses (supporting both bech32 and hex formats) and uint256 amounts.
Let's verify the function implementation in the source contract:
✅ Verification successful
Implementation verified - function correctly uses sendToBank
The justPrecompileSend function implementation matches its ABI and correctly uses FUNTOKEN_PRECOMPILE.sendToBank with proper parameter handling and amount verification, aligning with the PR's objectives of refactoring bankSend to sendToBank.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of justPrecompileSend matches its ABI
# Expected: Function should use FUNTOKEN_PRECOMPILE.sendToBank instead of bankSend
# Search for the function implementation
ast-grep --pattern 'function justPrecompileSend(string memory $_, uint256 $_) {
$$$
}'
Length of output: 90
Script:
#!/bin/bash
# Let's try to find the contract source file first
fd "TestNativeSendThenPrecompileSend.sol" --type f
# Then we can read its content
fd "TestNativeSendThenPrecompileSend.sol" --type f --exec cat {}
Length of output: 1921
eth/rpc/rpcapi/eth_api_test.go (1)
109-113: LGTM: Reasonable block number assertion tolerance.
The relaxed assertion allowing for a block difference of up to 2 blocks is a good approach to handle potential minor synchronization delays between the network and Ethereum client.
x/evm/keeper/funtoken_from_coin_test.go (4)
237-238: Consistent use of gas limit constant improves maintainability.
The introduction of evmtest.FunTokenGasLimitSendToEvm constant across multiple test cases ensures consistent gas limits and makes future adjustments easier.
Also applies to: 265-266, 605-605
Line range hint 359-401: Comprehensive balance assertions enhance test reliability.
The new balance assertions thoroughly verify the state after operations:
- Alice's balance after transfers
- Test contract's remaining balance
- Module account's escrowed balance
This provides better coverage of the state transitions and helps catch potential race conditions.
447-448: Well-documented test scenarios improve maintainability.
The improved documentation clearly explains:
- Initial state
- Contract operations
- Expected outcomes
- Potential race conditions being tested
This makes the test cases more maintainable and helps other developers understand the test coverage.
Also applies to: 593-594
371-374: Verify decimal handling in transfer amounts.
The test uses different decimal representations:
- Wei units for native transfers
- 6 decimals for precompile operations
Consider adding assertions to explicitly verify that the decimal conversions are handled correctly.
Also applies to: 503-503, 610-610
✅ Verification successful
Based on the search results, I can now generate the final response since I have sufficient information about decimal handling across the codebase.
Decimal handling is correctly implemented and consistently used across transfers
The codebase shows consistent decimal handling patterns:
- Native EVM transfers correctly use
NativeToWei()which adds 12 decimal places (multiplies by 10^12) to convert from micronibi (6 decimals) to wei (18 decimals) - Bank/precompile operations consistently use 6 decimal places (micronibi)
- The test correctly verifies both representations:
newSendAmtEvmTransferusesNativeToWei()for EVM native transfersnewSendAmtSendToBankuses direct micronibi values for bank operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent decimal handling across the codebase
rg -A 2 "NativeToWei|NewInt\([0-9]+e[0-9]+\)"
Length of output: 14348
x/evm/embeds/contracts/IFunToken.sol (4)
4-5: Constants are appropriately defined
Defining FUNTOKEN_PRECOMPILE_ADDRESS and FUNTOKEN_PRECOMPILE as constants enhances code readability and maintainability.
18-21: Optimized parameter passing using calldata
Changing the to parameter to string calldata from string memory improves gas efficiency for external calls.
24-27: Struct NibiruAccount is appropriately defined
The NibiruAccount struct effectively encapsulates both the Ethereum address and the Bech32 address, enhancing code clarity.
28-31: Struct FunToken is appropriately defined
The FunToken struct encapsulates the ERC20 address and the bank denomination, improving the interface's usability.
x/evm/keeper/msg_server.go (2)
369-371: Ensure correct refund quotient calculation
The code sets refundQuotient based on the fullRefundLeftoverGas flag:
refundQuotient := params.RefundQuotientEIP3529
if fullRefundLeftoverGas {
refundQuotient = 1 // 100% refund
}Ensure that setting refundQuotient to 1 correctly results in a 100% gas refund for internal calls. This is critical to prevent users from receiving unintended gas refunds or overpaying for gas.
Would you like to verify the usage of refundQuotient throughout the codebase to confirm its correctness?
384-384:
Potential concern with gas calculation precision
The calculation of gasUsed uses math.LegacyNewDec:
gasUsed := math.LegacyNewDec(int64(temporaryGasUsed)).TruncateInt().Uint64()Converting gas values through decimal arithmetic and truncating might introduce precision errors, especially with large numbers. Ensure that this conversion does not lead to incorrect gas usage calculations.
Consider simplifying the calculation:
-gasUsed := math.LegacyNewDec(int64(temporaryGasUsed)).TruncateInt().Uint64()
+gasUsed := temporaryGasUsedWould you like assistance in verifying the impact of this change?
| uint256 sentAmount = FUNTOKEN_PRECOMPILE.sendToBank( | ||
| erc20, | ||
| precompileAmount, // micro-WNIBI | ||
| precompileRecipient | ||
| precompileAmount, // micro-WNIBI | ||
| precompileRecipient |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can generate the final response:
Method renaming has been consistently applied across the codebase
The only remaining instance of bankSend is in documentation (x/devgas/v1/README.md), while all implementation files correctly use sendToBank. The documentation reference should be updated for consistency.
- Update needed in:
x/devgas/v1/README.md
🔗 Analysis chain
LGTM! Verify consistent method renaming across the codebase.
The method renaming from bankSend to sendToBank is more intuitive and matches the PR objectives.
Let's verify the consistent renaming across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of 'bankSend' that might have been missed
# and confirm all occurrences have been updated to 'sendToBank'
echo "Checking for any remaining 'bankSend' instances:"
rg "bankSend"
echo -e "\nVerifying 'sendToBank' implementations:"
rg "sendToBank"
Length of output: 4729
| { | ||
| "inputs": [ | ||
| { | ||
| "internalType": "address", | ||
| "name": "who", | ||
| "type": "address" | ||
| }, | ||
| { | ||
| "internalType": "string", | ||
| "name": "bankDenom", | ||
| "type": "string" | ||
| } | ||
| ], | ||
| "name": "bankBalance", | ||
| "outputs": [ | ||
| { | ||
| "internalType": "uint256", | ||
| "name": "bankBalance", | ||
| "type": "uint256" | ||
| }, | ||
| { | ||
| "components": [ | ||
| { | ||
| "internalType": "address", | ||
| "name": "ethAddr", | ||
| "type": "address" | ||
| }, | ||
| { | ||
| "internalType": "string", | ||
| "name": "bech32Addr", | ||
| "type": "string" | ||
| } | ||
| ], | ||
| "internalType": "struct IFunToken.NibiruAccount", | ||
| "name": "whoAddrs", | ||
| "type": "tuple" | ||
| } | ||
| ], | ||
| "stateMutability": "nonpayable", | ||
| "type": "function" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider changing state mutability to view.
The bankBalance function is well-designed for querying bank balances with proper denomination support. However, since this is a read-only operation, it should be marked as view instead of nonpayable to save gas and better reflect its behavior.
| FunTokenMethod_sendToBank: true, | ||
| FunTokenMethod_balance: false, | ||
| FunTokenMethod_bankBalance: false, | ||
| FunTokenMethod_whoAmI: false, |
There was a problem hiding this comment.
💡 Codebase verification
Method names are consistent, but there's an outdated reference to bankSend
The method names are correctly implemented and consistent between the Solidity interface (IFunToken.sol) and Go implementation (funtoken.go), with sendToBank, balance, bankBalance, and whoAmI being used consistently. However, there is an outdated reference to the old method name bankSend in x/devgas/v1/README.md that should be updated to maintain consistency.
x/devgas/v1/README.md: UpdatebankSendtosendToBankin the documentation
🔗 Analysis chain
LGTM! Mutation flags are correctly set for the new FunToken methods.
The mutation flags accurately reflect each method's behavior:
sendToBank:trueas it modifies state by transferring tokensbalance,bankBalance,whoAmI:falseas they are read-only query methods
The naming is consistent with the PR objectives to implement balance queries and improve usability.
Let's verify the method names are consistent across the codebase:
Let me gather more specific information about the method names in the relevant files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify method names are consistent across Solidity interfaces and Go code
# Test: Search for old method name. Expect: No occurrences of 'bankSend'
rg -l 'bankSend'
# Test: Search for new method names. Expect: Consistent usage across files
rg -l 'sendToBank|balance\W|bankBalance|whoAmI'
Length of output: 2200
Script:
#!/bin/bash
# Check the interface definition in Solidity
cat x/evm/embeds/contracts/IFunToken.sol
# Check the implementation in Go
rg -A 1 'func.*(?:sendToBank|balance\W|bankBalance|whoAmI)' x/evm/precompile/funtoken.go
# Check for any remaining references to old method name
rg -C 2 'bankSend' x/devgas/v1/README.md
Length of output: 2704
| s.Run("IFunToken.bankBalance", func() { | ||
| s.Require().NotEmpty(funtoken.BankDenom) | ||
| evmResp, err := deps.EvmKeeper.CallContract( | ||
| deps.Ctx, | ||
| embeds.SmartContract_FunToken.ABI, | ||
| deps.Sender.EthAddr, | ||
| &precompile.PrecompileAddr_FunToken, | ||
| false, | ||
| keeper.Erc20GasLimitExecute, | ||
| "bankBalance", | ||
| []any{ | ||
| deps.Sender.EthAddr, // who | ||
| funtoken.BankDenom, // bankDenom | ||
| }..., | ||
| ) | ||
| s.Require().NoError(err, evmResp) | ||
|
|
||
| bal, ethAddr, bech32Addr, err := new(FunTokenBankBalanceReturn).ParseFromResp(evmResp) | ||
| s.NoError(err) | ||
| s.Require().Equal("69420", bal.String()) | ||
| s.Equal(deps.Sender.EthAddr.Hex(), ethAddr.Hex()) | ||
| s.Equal(deps.Sender.NibiruAddr.String(), bech32Addr) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance balance test coverage.
Consider adding these test scenarios:
- Account with zero balance
- Non-existent account
- Account with only ERC20 balance
- Account with only bank balance
Also applies to: 258-281
| addrEth, addrBech32, bankDenom, err := p.parseArgsBankBalance(args) | ||
| bankBal := p.evmKeeper.Bank.GetBalance(ctx, addrBech32, bankDenom).Amount.BigInt() | ||
|
|
There was a problem hiding this comment.
Check for errors after parsing arguments
In the bankBalance function, the error returned by parseArgsBankBalance is not checked before using the parsed arguments. This could lead to unexpected behavior or panics if an error occurs during argument parsing.
Apply this diff to fix the issue:
+ if err != nil {
+ err = ErrInvalidArgs(err)
+ return
+ }📝 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.
| addrEth, addrBech32, bankDenom, err := p.parseArgsBankBalance(args) | |
| bankBal := p.evmKeeper.Bank.GetBalance(ctx, addrBech32, bankDenom).Amount.BigInt() | |
| addrEth, addrBech32, bankDenom, err := p.parseArgsBankBalance(args) | |
| if err != nil { | |
| err = ErrInvalidArgs(err) | |
| return | |
| } | |
| bankBal := p.evmKeeper.Bank.GetBalance(ctx, addrBech32, bankDenom).Amount.BigInt() | |
| @@ -355,17 +355,20 @@ func (k *Keeper) ApplyEvmMsg(ctx sdk.Context, | |||
| return nil, evmObj, errors.Wrapf(core.ErrGasUintOverflow, "ApplyEvmMsg: message gas limit (%d) < leftover gas (%d)", msg.Gas(), leftoverGas) | |||
| } | |||
|
|
|||
| // TODO: UD-DEBUG: Clarify text below. | |||
There was a problem hiding this comment.
Remove debugger-specific comments before merging
The comment on line 358 includes "UD-DEBUG," which appears to be a personal note or a debugger-specific comment.
Personal notes or debug comments should be removed before merging to maintain code professionalism.
Apply this change:
-// TODO: UD-DEBUG: Clarify text below.
+// TODO: Clarify the following comments for better understanding.📝 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.
| // TODO: UD-DEBUG: Clarify text below. | |
| // TODO: Clarify the following comments for better understanding. |
Purpose / Abstract
This change implements two queries related to retrieving bank and ERC20 balances
from an EVM transaction. Prior to this change, there was no simple way to access
this information without using the gRPC query server.
Summary by CodeRabbit
Release Notes
New Features
balance,bankBalance, andwhoAmI.Bug Fixes
Documentation
Tests