x/evm/embeds/contracts/TestERC20TransferThenPrecompileSend.sol (1)

Line range hint 37-42: Consider improving error message formatting

The 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 construction

While 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 logic

Both 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:

1. Add NatSpec documentation to explain the function's purpose, parameters, and return values
2. 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 args parameter 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 the whoAmI function.

While the function successfully implements the address format conversion requirement, consider these improvements:

1. Change state mutability to view since this is a read-only operation
2. Consider a more descriptive name like getAddressFormats or convertAddress to better reflect its purpose

The 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 argsIdx while other functions use argIdx. 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 GetBalance method 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 parseArgsWasmExecuteMulti is 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_000 with 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 small

x/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 justPrecompileSend function 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 description

The 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: Add view modifier to the balance function

The balance function appears to be read-only and does not modify state. Adding the view modifier can optimize gas usage and clearly communicate the function's intent.

Apply this diff to add the view modifier:

    function balance(
        address who,
        address funtoken
    )
-       external
+       external view
        returns (
            uint256 erc20Balance,
            uint256 bankBalance,
            FunToken memory token,
            NibiruAccount memory whoAddrs
        );

---

45-48: Add view modifier to the bankBalance function

Since the bankBalance function likely does not modify state, adding the view modifier optimizes gas usage and denotes its read-only nature.

Apply this diff to add the view modifier:

    function bankBalance(
        address who,
        string calldata bankDenom
    )
-       external
+       external view
        returns (uint256 bankBalance, NibiruAccount memory whoAddrs);

---

50-52: Add view modifier to the whoAmI function

The whoAmI function seems to be a read-only function that does not modify state. Adding the view modifier indicates its read-only status and can improve gas efficiency.

Apply this diff to add the view modifier:

    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 changes

The sendToBank function 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 duplication

The argument parsing functions (parseArgsSendToBank, parseArgsBalance, parseArgsBankBalance, and parseArgsWhoAmI) 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 balance

In the balance function, when retrieving the bank balance using GetBalance, consider handling any potential errors that might occur. While GetBalance usually 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 comments

The 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 comments

The 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 formatting

There 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 function

The 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 ApplyEvmMsg function cleaner and the gas refund logic reusable.

---

365-369: Correct grammatical error in comments

There's a grammatical error in the comment on line 367:

"... if the ApplyEvmMsg call originated from a state transition where the chain set the gas limit and not an end-user."

Consider rephrasing for clarity:

"... if the ApplyEvmMsg call originated from a state transition where the chain sets the gas limit, not an end-user."

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/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

#!/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$"

#!/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"

#!/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 $_) {
  $$$
}'

#!/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 {}

#!/bin/bash
# Check for consistent decimal handling across the codebase
rg -A 2 "NativeToWei|NewInt\([0-9]+e[0-9]+\)" 

refundQuotient := params.RefundQuotientEIP3529
if fullRefundLeftoverGas {
    refundQuotient = 1 // 100% refund
}

gasUsed := math.LegacyNewDec(int64(temporaryGasUsed)).TruncateInt().Uint64()

-gasUsed := math.LegacyNewDec(int64(temporaryGasUsed)).TruncateInt().Uint64()
+gasUsed := temporaryGasUsed