feat(evm): add "eth.evm.v1.MsgConvertEvmToCoin" tx for ERC20 to bank coin conversions with a non-Ethereum transaction #2345
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds ERC20→bank-coin conversion: new protobuf messages/events and RPC; keeper handlers for coin/ERC20/WNIBI origins; CLI and tests; params extended with CanonicalWnibi and upgrade wiring; CallContractWithInput gains optional value param and many call sites/tests updated; embeds WNIBI contract and sudo-based params auth. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User/CLI
participant CLI as x/evm CLI
participant MsgSrv as Msg gRPC
participant Keeper as EVM Keeper
participant EVM as EVM / StateDB
participant ERC20 as ERC20 Contract
participant Bank as Cosmos Bank
U->>CLI: convert-evm-to-coin <erc20> <amount> <to>
CLI->>MsgSrv: Broadcast MsgConvertEvmToCoin
MsgSrv->>Keeper: ConvertEvmToCoin(msg)
Keeper->>Keeper: Validate msg (sender, erc20, amount, to)
alt erc20 == Params.CanonicalWnibi
Keeper->>EVM: Call withdraw on WNIBI (CallContractWithInput(value=nil))
EVM-->>Keeper: result & logs
Keeper->>Bank: send bank coins to recipient
else coin-originated mapping
Keeper->>ERC20: burnFromAuthority(amount)
ERC20-->>Keeper: logs/result
Keeper->>Bank: transfer bank coins to recipient
else erc20-originated mapping
Keeper->>ERC20: transfer from sender to module
ERC20-->>Keeper: logs/result
Keeper->>Bank: mint+send bank coins to recipient
end
Keeper-->>MsgSrv: response + events (EventConvertEvmToCoin, tx logs)
MsgSrv-->>CLI: tx result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a new feature that enables converting ERC20 tokens back to their corresponding bank coin representation. The implementation includes comprehensive updates across protobuf definitions, gRPC interfaces, CLI commands, message validation, and keeper logic to support bidirectional token conversion.
- Added
ConvertEvmToCoinfunctionality to convert ERC20 tokens to bank coins - Implemented proper validation, event emission, and test coverage for the new conversion feature
- Fixed a spelling error in oracle parameter documentation and updated cosmos-sdk dependency
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/eth/evm/v1/tx.proto | Added MsgConvertEvmToCoin message and service definition |
| proto/eth/evm/v1/events.proto | Added EventConvertEvmToCoin event for conversion tracking |
| x/evm/keeper/msg_server.go | Implemented conversion logic for both coin-originated and ERC20-originated tokens |
| x/evm/msg.go | Added message validation and signing methods for MsgConvertEvmToCoin |
| x/evm/cli/tx.go | Added CLI command for ERC20 to bank coin conversion |
| x/oracle/types/oracle.pb.go | Fixed spelling error: "maxium" to "maximum" |
| go.mod | Updated cosmos-sdk dependency to v0.47.11-nibiru.3 |
x/evm/keeper/msg_server.go
Outdated
| From: evm.EVM_MODULE_ADDRESS, | ||
| Nonce: k.GetAccNonce(ctx, evm.EVM_MODULE_ADDRESS), |
There was a problem hiding this comment.
The EVM module address should not be the sender for burnFromAuthority call on coin-originated tokens. The actual sender should be used as the From address to properly represent who is initiating the burn operation.
| From: evm.EVM_MODULE_ADDRESS, | |
| Nonce: k.GetAccNonce(ctx, evm.EVM_MODULE_ADDRESS), | |
| From: senderEthAddr, | |
| Nonce: k.GetAccNonce(ctx, senderEthAddr), |
Co-authored-by: Copilot <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.gitignore (1)
362-362: Preserve trailing newline to avoid unnecessary future diffsRemoving the final newline violates POSIX text-file conventions and can cause “No newline at end of file” diff noise going forward. Consider keeping the terminating line break.
debug_container.dot +api/nibiru/oracle/v1/oracle.pulsar.go (1)
3794-3803: Residual typo in comment (“deviation.a”)The words “deviation” and “a” are concatenated, which harms readability and documentation searchability.
-// deviation, the band is taken to be 1 standard deviation.a price +// deviation, the band is taken to be 1 standard deviation.No functional impact, just polish.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
api/eth/evm/v1/tx_grpc.pb.gois excluded by!**/*.pb.gox/evm/events.pb.gois excluded by!**/*.pb.gox/evm/tx.pb.gois excluded by!**/*.pb.gox/oracle/types/oracle.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
.gitignore(1 hunks)CHANGELOG.md(1 hunks)api/eth/evm/v1/events.pulsar.go(6 hunks)api/eth/evm/v1/tx.pulsar.go(6 hunks)api/nibiru/oracle/v1/oracle.pulsar.go(1 hunks)go.mod(1 hunks)proto/eth/evm/v1/events.proto(1 hunks)proto/eth/evm/v1/tx.proto(2 hunks)x/evm/cli/cli_test.go(1 hunks)x/evm/cli/tx.go(2 hunks)x/evm/codec.go(1 hunks)x/evm/keeper/funtoken_evm_to_coin_test.go(1 hunks)x/evm/keeper/msg_server.go(1 hunks)x/evm/msg.go(2 hunks)x/evm/msg_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2315
File: app/upgrades/v2_5_0/v2_5_0.go:0-0
Timestamp: 2025-05-21T21:47:04.451Z
Learning: In the v2.5.0 upgrade handler for Nibiru Chain, excess stNIBI ERC20 token balances (those not belonging to explicitly tracked holders) are sent to a Nibiru team-controlled Gnosis Safe for redistribution to ensure no tokens are lost during contract upgrades.
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2095
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-10-31T18:28:26.466Z
Learning: When implementing the `bankkeeper.Keeper` interface in `x/evm/keeper/bank_extension.go`, methods must use non-pointer receivers to satisfy the interface requirements.
go.mod (2)
Learnt from: Unique-Divine
PR: #2315
File: app/upgrades/v2_5_0/v2_5_0.go:0-0
Timestamp: 2025-05-21T21:47:04.451Z
Learning: In the v2.5.0 upgrade handler for Nibiru Chain, excess stNIBI ERC20 token balances (those not belonging to explicitly tracked holders) are sent to a Nibiru team-controlled Gnosis Safe for redistribution to ensure no tokens are lost during contract upgrades.
Learnt from: CalicoNino
PR: #2317
File: token-registry/official_erc20s.json:31-31
Timestamp: 2025-05-30T17:11:36.101Z
Learning: In the Nibiru token registry, the USDC (Stargate) token intentionally uses different symbols in different files: "USDC.e" in token-registry/official_erc20s.json and "USDC" in token-registry/cosmos_assetlist.go. This difference is deliberate and should not be flagged as an inconsistency.
CHANGELOG.md (1)
Learnt from: Unique-Divine
PR: #2315
File: app/upgrades/v2_5_0/v2_5_0.go:0-0
Timestamp: 2025-05-21T21:47:04.451Z
Learning: In the v2.5.0 upgrade handler for Nibiru Chain, excess stNIBI ERC20 token balances (those not belonging to explicitly tracked holders) are sent to a Nibiru team-controlled Gnosis Safe for redistribution to ensure no tokens are lost during contract upgrades.
proto/eth/evm/v1/events.proto (1)
Learnt from: Unique-Divine
PR: #2095
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-10-31T18:28:26.466Z
Learning: When implementing the bankkeeper.Keeper interface in x/evm/keeper/bank_extension.go, methods must use non-pointer receivers to satisfy the interface requirements.
x/evm/codec.go (1)
Learnt from: onikonychev
PR: #2101
File: eth/rpc/backend/tx_info.go:144-170
Timestamp: 2024-10-31T16:39:44.773Z
Learning: In the file eth/rpc/backend/tx_info.go, when implementing the MarshalJSON method for TransactionReceipt, using a custom struct for marshaling instead of an intermediate map[string]interface{} does not work.
x/evm/cli/tx.go (2)
Learnt from: Unique-Divine
PR: #1869
File: x/evm/cli/cli.go:44-47
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The loop in GetQueryCmd function of x/evm/cli/cli.go is a placeholder for future subcommands.
Learnt from: Unique-Divine
PR: #1869
File: x/evm/cli/cli.go:44-47
Timestamp: 2024-07-27T01:11:54.422Z
Learning: The loop in GetQueryCmd function of x/evm/cli/cli.go is a placeholder for future subcommands.
x/evm/cli/cli_test.go (5)
Learnt from: Unique-Divine
PR: #2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in x/evm/keeper/bank_extension_test.go, prefer comparing gas consumption values as formatted strings using fmt.Sprintf("%d", value) instead of numeric values directly in s.Equalf. This avoids hexadecimal representations that are hard to read in shell outputs.
Learnt from: Unique-Divine
PR: #2119
File: x/evm/keeper/bank_extension_test.go:161-165
Timestamp: 2024-12-06T16:09:01.767Z
Learning: In Go test assertions within x/evm/keeper/bank_extension_test.go, when comparing uint64 values using s.Equalf, prefer formatting the values as decimal strings with fmt.Sprintf("%d", value) to avoid them being printed in hexadecimal.
Learnt from: k-yang
PR: #2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The NewEVM() method in TestDeps returns only *vm.EVM without an error value.
Learnt from: Unique-Divine
PR: #1869
File: x/evm/cli/cli.go:44-47
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The loop in GetQueryCmd function of x/evm/cli/cli.go is a placeholder for future subcommands.
Learnt from: Unique-Divine
PR: #1869
File: x/evm/cli/cli.go:44-47
Timestamp: 2024-07-27T01:11:54.422Z
Learning: The loop in GetQueryCmd function of x/evm/cli/cli.go is a placeholder for future subcommands.
x/evm/msg_test.go (5)
Learnt from: Unique-Divine
PR: #2119
File: x/evm/keeper/bank_extension_test.go:161-165
Timestamp: 2024-12-06T16:09:01.767Z
Learning: In Go test assertions within x/evm/keeper/bank_extension_test.go, when comparing uint64 values using s.Equalf, prefer formatting the values as decimal strings with fmt.Sprintf("%d", value) to avoid them being printed in hexadecimal.
Learnt from: Unique-Divine
PR: #2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in x/evm/keeper/bank_extension_test.go, prefer comparing gas consumption values as formatted strings using fmt.Sprintf("%d", value) instead of numeric values directly in s.Equalf. This avoids hexadecimal representations that are hard to read in shell outputs.
Learnt from: Unique-Divine
PR: #1680
File: x/perp/v2/keeper/msg_server.go:210-210
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The user indicated that error handling for sdk.AccAddressFromBech32(msg.Sender) is covered by prior ValidateBasic checks in the code flow.
Learnt from: Unique-Divine
PR: #1680
File: x/perp/v2/keeper/msg_server.go:210-210
Timestamp: 2024-07-27T01:11:54.422Z
Learning: The user indicated that error handling for sdk.AccAddressFromBech32(msg.Sender) is covered by prior ValidateBasic checks in the code flow.
Learnt from: Unique-Divine
PR: #1680
File: x/perp/v2/keeper/msg_server.go:198-198
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The user Unique-Divine has indicated that the error check for sdk.AccAddressFromBech32(msg.Sender) is not necessary in the context of their code because it is covered by the ValidateBasic method.
x/evm/msg.go (3)
Learnt from: Unique-Divine
PR: #1680
File: x/perp/v2/keeper/msg_server.go:210-210
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The user indicated that error handling for sdk.AccAddressFromBech32(msg.Sender) is covered by prior ValidateBasic checks in the code flow.
Learnt from: Unique-Divine
PR: #1680
File: x/perp/v2/keeper/msg_server.go:210-210
Timestamp: 2024-07-27T01:11:54.422Z
Learning: The user indicated that error handling for sdk.AccAddressFromBech32(msg.Sender) is covered by prior ValidateBasic checks in the code flow.
Learnt from: Unique-Divine
PR: #1680
File: x/perp/v2/keeper/msg_server.go:198-198
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The user Unique-Divine has indicated that the error check for sdk.AccAddressFromBech32(msg.Sender) is not necessary in the context of their code because it is covered by the ValidateBasic method.
x/evm/keeper/funtoken_evm_to_coin_test.go (5)
Learnt from: Unique-Divine
PR: #2119
File: x/evm/keeper/bank_extension_test.go:161-165
Timestamp: 2024-12-06T16:09:01.767Z
Learning: In Go test assertions within x/evm/keeper/bank_extension_test.go, when comparing uint64 values using s.Equalf, prefer formatting the values as decimal strings with fmt.Sprintf("%d", value) to avoid them being printed in hexadecimal.
Learnt from: Unique-Divine
PR: #2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in x/evm/keeper/bank_extension_test.go, prefer comparing gas consumption values as formatted strings using fmt.Sprintf("%d", value) instead of numeric values directly in s.Equalf. This avoids hexadecimal representations that are hard to read in shell outputs.
Learnt from: k-yang
PR: #2129
File: x/evm/embeds/contracts/TestInfiniteRecursionERC20.sol:24-32
Timestamp: 2024-12-26T04:43:43.966Z
Learning: The TestInfiniteRecursionERC20 contract is intentionally designed to test infinite recursion scenarios in ERC20-based contracts for the FunToken architecture.
Learnt from: Unique-Divine
PR: #2095
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-10-31T18:28:26.466Z
Learning: When implementing the bankkeeper.Keeper interface in x/evm/keeper/bank_extension.go, methods must use non-pointer receivers to satisfy the interface requirements.
Learnt from: Unique-Divine
PR: #2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In x/evm/keeper/bank_extension.go, assigning a new GasMeter to the context within the NibiruBankKeeper.ForceGasInvariant function is intentional and acceptable. This ensures that subsequent operations do not affect the parent ctx's gas consumption.
x/evm/keeper/msg_server.go (6)
Learnt from: Unique-Divine
PR: #2095
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-10-31T18:28:26.466Z
Learning: When implementing the bankkeeper.Keeper interface in x/evm/keeper/bank_extension.go, methods must use non-pointer receivers to satisfy the interface requirements.
Learnt from: Unique-Divine
PR: #2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In x/evm/keeper/bank_extension.go, assigning a new GasMeter to the context within the NibiruBankKeeper.ForceGasInvariant function is intentional and acceptable. This ensures that subsequent operations do not affect the parent ctx's gas consumption.
Learnt from: Unique-Divine
PR: #2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the ApplyEvmTx function within x/evm/keeper/msg_server.go, a zero value for weiPerGas is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.
Learnt from: Unique-Divine
PR: #2119
File: x/evm/keeper/bank_extension_test.go:161-165
Timestamp: 2024-12-06T16:09:01.767Z
Learning: In Go test assertions within x/evm/keeper/bank_extension_test.go, when comparing uint64 values using s.Equalf, prefer formatting the values as decimal strings with fmt.Sprintf("%d", value) to avoid them being printed in hexadecimal.
Learnt from: Unique-Divine
PR: #2097
File: x/oracle/keeper/keeper.go:195-195
Timestamp: 2024-10-31T09:55:25.630Z
Learning: In x/oracle/keeper/keeper.go, within the GetDatedExchangeRate method, it's not possible for exchangeRate.CreatedBlock to be in the future relative to ctx.BlockHeight(), so additional validation before time calculation is unnecessary.
Learnt from: Unique-Divine
PR: #1687
File: x/perp/v2/keeper/amm.go:0-0
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The user has addressed the issue of explicitly handling the case where costAmt is zero in the function handleMarketUpdateCost in the file x/perp/v2/keeper/amm.go.
api/eth/evm/v1/events.pulsar.go (2)
Learnt from: onikonychev
PR: #2101
File: eth/rpc/backend/tx_info.go:144-170
Timestamp: 2024-10-31T16:39:44.773Z
Learning: In the file eth/rpc/backend/tx_info.go, when implementing the MarshalJSON method for TransactionReceipt, using a custom struct for marshaling instead of an intermediate map[string]interface{} does not work.
Learnt from: Unique-Divine
PR: #2095
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-10-31T18:28:26.466Z
Learning: When implementing the bankkeeper.Keeper interface in x/evm/keeper/bank_extension.go, methods must use non-pointer receivers to satisfy the interface requirements.
api/eth/evm/v1/tx.pulsar.go (1)
Learnt from: onikonychev
PR: #2101
File: eth/rpc/backend/tx_info.go:144-170
Timestamp: 2024-10-31T16:39:44.773Z
Learning: In the file eth/rpc/backend/tx_info.go, when implementing the MarshalJSON method for TransactionReceipt, using a custom struct for marshaling instead of an intermediate map[string]interface{} does not work.
🧬 Code Graph Analysis (3)
x/evm/codec.go (2)
api/eth/evm/v1/tx.pulsar.go (9)
MsgCreateFunToken(9305-9316)MsgCreateFunToken(9331-9331)MsgCreateFunToken(9334-9336)MsgConvertCoinToEvm(9396-9407)MsgConvertCoinToEvm(9422-9422)MsgConvertCoinToEvm(9425-9427)MsgConvertEvmToCoin(9477-9491)MsgConvertEvmToCoin(9506-9506)MsgConvertEvmToCoin(9509-9511)x/evm/tx.pb.go (9)
MsgCreateFunToken(480-487)MsgCreateFunToken(491-491)MsgCreateFunToken(492-494)MsgConvertCoinToEvm(582-589)MsgConvertCoinToEvm(593-593)MsgConvertCoinToEvm(594-596)MsgConvertEvmToCoin(675-685)MsgConvertEvmToCoin(689-689)MsgConvertEvmToCoin(690-692)
x/evm/cli/cli_test.go (3)
x/evm/cli/cli_setup_test.go (2)
Suite(23-32)TestCase(66-71)x/evm/keeper/grpc_query_test.go (1)
TestCase(23-33)app/server/flags.go (1)
Address(13-13)
x/evm/msg_test.go (2)
x/evm/tx.pb.go (3)
MsgConvertEvmToCoin(675-685)MsgConvertEvmToCoin(689-689)MsgConvertEvmToCoin(690-692)eth/eip55.go (1)
EIP55Addr(16-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: e2e-evm
- GitHub Check: integration-tests
- GitHub Check: lint
🔇 Additional comments (26)
go.mod (1)
16-16: LGTM: Dependency update aligns with new EVM features.The update to the custom Nibiru fork of cosmos-sdk (
v0.47.11-nibiru.3) is appropriate for supporting the newconvertEvmToCoinfunctionality introduced in this PR.x/evm/codec.go (1)
37-39: LGTM: Proper codec registration for new message types.The registration of
MsgCreateFunToken,MsgConvertCoinToEvm, andMsgConvertEvmToCoinassdk.Msgimplementations follows the established pattern and is necessary for protobuf Any packing/unpacking functionality.proto/eth/evm/v1/events.proto (1)
79-88: LGTM: Well-structured event message for EVM to coin conversion.The
EventConvertEvmToCoinmessage follows the established pattern of existing event messages and includes appropriate field types and gogoproto annotations. Thenullable = falseannotation on thebank_coinfield is correct since it should always contain a value.x/evm/cli/tx.go (2)
31-31: LGTM: Proper registration of the new CLI command.The
CmdConvertEvmToCoin()command is correctly added to the commands list alongside existing transaction commands.
128-172: LGTM: Well-implemented CLI command with proper validation.The
CmdConvertEvmToCoincommand is well-structured with:
- Proper argument validation for all three parameters
- Descriptive error messages for better user experience
- Consistent pattern with existing CLI commands
- Appropriate use of SDK validation functions
The validation chain ensures:
- ERC20 address format is valid (EIP-55)
- Amount is parseable as a valid integer
- Recipient address is a valid bech32 account address
x/evm/cli/cli_test.go (1)
52-124: Excellent comprehensive test coverage for the new CLI command.The test function
TestCmdConvertEvmToCoinprovides thorough coverage of the newconvert-evm-to-coincommand with appropriate test cases for both success and failure scenarios. The implementation follows the established patterns in the codebase and uses proper dummy data.x/evm/msg_test.go (2)
999-1123: Comprehensive validation testing for MsgConvertEvmToCoin.The
TestMsgConvertEvmToCoin_ValidateBasictest provides excellent coverage of all validation scenarios including invalid addresses, empty fields, and invalid amounts. The test structure is consistent with existing patterns in the codebase.
1125-1137: Clean and correct GetSigners test implementation.The test properly verifies that the
GetSignersmethod returns exactly one signer corresponding to the sender address, following the established pattern for message signer testing.x/evm/msg.go (2)
37-38: Correct interface assertions for both conversion message types.The sdk.Msg interface assertions are properly added for both
MsgConvertCoinToEvmandMsgConvertEvmToCoin, ensuring compile-time interface compliance.
526-556: Well-implemented message interface methods.The implementation of
GetSigners,ValidateBasic, andGetSignBytesmethods forMsgConvertEvmToCoinfollows established patterns in the codebase:
GetSignerscorrectly extracts the sender address (error handling is omitted as per established pattern whereValidateBasichandles validation)ValidateBasicprovides comprehensive validation of all fields with appropriate error messagesGetSignBytesuses the standard JSON marshaling approachproto/eth/evm/v1/tx.proto (2)
37-40: Clean RPC method definition following established patterns.The
ConvertEvmToCoinRPC method is properly defined with clear documentation, mirroring the structure of the existingConvertCoinToEvmmethod for consistency.
294-315: Well-structured message definitions with appropriate field types.The
MsgConvertEvmToCoinmessage is properly defined with:
- Clear field documentation
- Appropriate custom types for EIP55 addresses and math integers
- Correct field numbering and protobuf annotations
- Empty response message following established patterns
api/eth/evm/v1/tx.pulsar.go (6)
7668-7943: Auto-generated protobuf code looks correctThe FastReflection implementation for
MsgConvertEvmToCoinfollows the expected protobuf code generation patterns with proper handling of all four fields.
9476-9565: Message type definitions are well-structuredThe
MsgConvertEvmToCoinmessage properly defines all required fields for ERC20 to bank coin conversion, and the empty response type follows standard patterns where results are communicated via events.
9725-9781: Auto-generated protobuf descriptor data
9796-9796: Message type array correctly sizedThe array size update to 14 properly accounts for the two new message types.
9810-9839: Type registrations and RPC mappings are correctThe new message types are properly registered and the
ConvertEvmToCoinRPC method is correctly mapped with appropriate input/output types.
9995-10029: Reflection exporters properly implementedThe exporters for the new message types follow standard protobuf patterns and the message count is correctly updated.
x/evm/keeper/funtoken_evm_to_coin_test.go (4)
30-159: Well-structured test coverage for coin-originated token conversions!The test comprehensively covers the conversion flow for coin-originated FunTokens, including:
- Proper setup with bank metadata and FunToken creation
- Happy path conversion with balance assertions
- Error cases for insufficient balance and non-existent mappings
- Correct use of fresh EVM instances for state assertions
161-319: Excellent test coverage for ERC20-originated token conversions!The test properly validates:
- ERC20 token deployment and FunToken creation
- Required approval mechanism before conversion
- Balance tracking for sender, recipient, and EVM module
- Failure case when approval is missing
The approval requirement is critical for security and is well-tested here.
385-406: Good event verification approach with proper attribute checking!The event verification correctly:
- Iterates through events to find the specific event type
- Parses attributes into a map for easy verification
- Handles quoted attribute values appropriately
- Validates all expected attributes including serialized bank_coin
The commented EventTxLog check with explanation is acceptable given the timing issues mentioned.
418-501: Comprehensive test for multiple recipient conversions!The test effectively validates:
- Sequential conversions to different recipients
- Correct balance updates for each recipient
- Accurate remaining ERC20 balance calculation
- Proper use of fresh EVM instance for state verification
x/evm/keeper/msg_server.go (3)
756-792: Clean implementation of the main conversion handler!The method properly:
- Validates addresses using MustAccAddressFromBech32
- Ensures exactly one FunToken mapping exists
- Correctly dispatches based on token origin
- Follows the established pattern from ConvertCoinToEvm
794-882: Correct implementation for coin-originated token conversion!The method properly:
- Burns ERC20 tokens using
burnFromAuthority(appropriate for module-owned contracts)- Transfers equivalent bank coins from module to recipient
- Manages StateDB lifecycle with proper cleanup
- Emits both conversion event and EVM logs
884-978: Proper handling of ERC20-originated token conversion!The method correctly:
- Transfers ERC20 tokens to EVM module using
transferFrom- Mints equivalent bank coins to maintain supply invariant
- Sends minted coins to recipient
- Manages StateDB and emits appropriate events
Note: This implementation requires the user to have approved the EVM module to spend their tokens before calling this method. The approval check happens within the ERC20 contract's
transferFrommethod.api/eth/evm/v1/events.pulsar.go (1)
1-5874: Skip reviewing auto-generated protobuf filesThis file is auto-generated from protobuf definitions and should not be manually edited. Any changes should be made to the source
.protofiles instead.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
x/evm/keeper/msg_server.go (2)
823-824: Consider the sender address in the EVM message.Based on past feedback, the EVM module address should represent who is executing the burn operation, while the actual token holder (senderEthAddr) is specified as the burn target in the contract call parameters. The current implementation appears correct as the EVM module is authorized to burn tokens from any holder for coin-originated tokens.
928-967: Verify the transferFrom pattern for ERC20-originated tokens.The implementation uses
transferFromafter checking allowance, which is a standard ERC20 pattern. However, this requires users to first approve the EVM module before calling the conversion function.Consider if this two-step process (approve + convert) aligns with the intended UX, or if there's a need for a different approach like having users call
transferdirectly to the EVM module address.The current pattern requires:
- User calls
approve(EVM_MODULE_ADDRESS, amount)on the ERC20 contract- User calls
ConvertEvmToCoinmessage- EVM module calls
transferFrom(user, module, amount)Please confirm this is the intended flow for ERC20-originated token conversions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/evm/keeper/msg_server.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2315
File: app/upgrades/v2_5_0/v2_5_0.go:0-0
Timestamp: 2025-05-21T21:47:04.451Z
Learning: In the v2.5.0 upgrade handler for Nibiru Chain, excess stNIBI ERC20 token balances (those not belonging to explicitly tracked holders) are sent to a Nibiru team-controlled Gnosis Safe for redistribution to ensure no tokens are lost during contract upgrades.
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2095
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-10-31T18:28:26.466Z
Learning: When implementing the `bankkeeper.Keeper` interface in `x/evm/keeper/bank_extension.go`, methods must use non-pointer receivers to satisfy the interface requirements.
x/evm/keeper/msg_server.go (8)
Learnt from: Unique-Divine
PR: #2095
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-10-31T18:28:26.466Z
Learning: When implementing the bankkeeper.Keeper interface in x/evm/keeper/bank_extension.go, methods must use non-pointer receivers to satisfy the interface requirements.
Learnt from: Unique-Divine
PR: #2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In x/evm/keeper/bank_extension.go, assigning a new GasMeter to the context within the NibiruBankKeeper.ForceGasInvariant function is intentional and acceptable. This ensures that subsequent operations do not affect the parent ctx's gas consumption.
Learnt from: Unique-Divine
PR: #2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the ApplyEvmTx function within x/evm/keeper/msg_server.go, a zero value for weiPerGas is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.
Learnt from: Unique-Divine
PR: #2119
File: x/evm/keeper/bank_extension_test.go:161-165
Timestamp: 2024-12-06T16:09:01.767Z
Learning: In Go test assertions within x/evm/keeper/bank_extension_test.go, when comparing uint64 values using s.Equalf, prefer formatting the values as decimal strings with fmt.Sprintf("%d", value) to avoid them being printed in hexadecimal.
Learnt from: Unique-Divine
PR: #1687
File: x/perp/v2/keeper/amm.go:0-0
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The user has addressed the issue of explicitly handling the case where costAmt is zero in the function handleMarketUpdateCost in the file x/perp/v2/keeper/amm.go.
Learnt from: k-yang
PR: #2093
File: x/evm/precompile/funtoken.go:85-87
Timestamp: 2024-11-04T20:38:45.909Z
Learning: In the Run methods of precompiled contracts in x/evm/precompile/, after handling errors from OnRunStart, it's unnecessary to check if err is nil before consuming gas with contract.UseGas(), as err will always be nil at that point.
Learnt from: onikonychev
PR: #2150
File: x/evm/embeds/contracts/TestERC20MaliciousTransfer.sol:36-38
Timestamp: 2025-01-31T07:16:54.084Z
Learning: Mint functions in ERC20 tokens should have access control (e.g., using OpenZeppelin's Ownable) to prevent unauthorized minting. The function should check for owner privileges using require(msg.sender == owner(), "must be owner").
Learnt from: Unique-Divine
PR: #1680
File: x/perp/v2/keeper/msg_server.go:198-198
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The user Unique-Divine has indicated that the error check for sdk.AccAddressFromBech32(msg.Sender) is not necessary in the context of their code because it is covered by the ValidateBasic method.
🪛 GitHub Check: e2e-evm
x/evm/keeper/msg_server.go
[failure] 924-924:
approvalResp.ReturnData undefined (type *evm.MsgEthereumTxResponse has no field or method ReturnData)
[failure] 912-912:
undefined: evmMsg
🪛 GitHub Check: unit-tests
x/evm/keeper/msg_server.go
[failure] 924-924:
approvalResp.ReturnData undefined (type *evm.MsgEthereumTxResponse has no field or method ReturnData)
[failure] 912-912:
undefined: evmMsg
[failure] 924-924:
approvalResp.ReturnData undefined (type *evm.MsgEthereumTxResponse has no field or method ReturnData)
[failure] 912-912:
undefined: evmMsg
[failure] 924-924:
approvalResp.ReturnData undefined (type *evm.MsgEthereumTxResponse has no field or method ReturnData)
[failure] 912-912:
undefined: evmMsg
🪛 GitHub Check: build
x/evm/keeper/msg_server.go
[failure] 924-924:
approvalResp.ReturnData undefined (type *evm.MsgEthereumTxResponse has no field or method ReturnData)
[failure] 912-912:
undefined: evmMsg
🪛 GitHub Actions: Unit Tests
x/evm/keeper/msg_server.go
[error] 912-912: Compilation error: undefined identifier 'evmMsg' at line 912.
🪛 GitHub Actions: EVM E2E tests
x/evm/keeper/msg_server.go
[error] 912-912: undefined: evmMsg
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-tests
🔇 Additional comments (1)
x/evm/keeper/msg_server.go (1)
756-792: LGTM! Well-structured conversion entry point.The main conversion function correctly handles address parsing, FunToken lookup validation, and delegates appropriately to specialized handlers based on token origin. The error handling for missing or duplicate FunToken mappings is comprehensive.
This reverts commit 335862b.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2345 +/- ##
==========================================
+ Coverage 56.51% 56.65% +0.13%
==========================================
Files 330 334 +4
Lines 26425 27154 +729
==========================================
+ Hits 14935 15385 +450
- Misses 10461 10701 +240
- Partials 1029 1068 +39
🚀 New features to boost your workflow:
|
k-yang
left a comment
k-yang
left a comment
There was a problem hiding this comment.
Left a comment on how to skip the approve step
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
CHANGELOG.md (1)
52-52: Fix the typo in the GitHub URL for PR #2345.
gittub.comshould begithub.comto avoid a broken link.- - [#2345](https://gittub.com/NibiruChain/nibiru/pull/2345) - feat(evm): add convertEvmToCoin functions + - [#2345](https://github.com/NibiruChain/nibiru/pull/2345) - feat(evm): add convertEvmToCoin functions
🧹 Nitpick comments (1)
CHANGELOG.md (1)
43-47: Stray tilde markers left in comment block.The trailing
~after lines inside the HTML comment appears to be a leftover diff marker and should be removed to keep the comment clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(0 hunks)CHANGELOG.md(1 hunks)token-registry/official_erc20s.json(1 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
✅ Files skipped from review due to trivial changes (1)
- token-registry/official_erc20s.json
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2315
File: app/upgrades/v2_5_0/v2_5_0.go:0-0
Timestamp: 2025-05-21T21:47:04.451Z
Learning: In the v2.5.0 upgrade handler for Nibiru Chain, excess stNIBI ERC20 token balances (those not belonging to explicitly tracked holders) are sent to a Nibiru team-controlled Gnosis Safe for redistribution to ensure no tokens are lost during contract upgrades.
CHANGELOG.md (1)
Learnt from: Unique-Divine
PR: #2315
File: app/upgrades/v2_5_0/v2_5_0.go:0-0
Timestamp: 2025-05-21T21:47:04.451Z
Learning: In the v2.5.0 upgrade handler for Nibiru Chain, excess stNIBI ERC20 token balances (those not belonging to explicitly tracked holders) are sent to a Nibiru team-controlled Gnosis Safe for redistribution to ensure no tokens are lost during contract upgrades.
🪛 LanguageTool
CHANGELOG.md
[style] ~55-~55: Replace ‘get’ with a more descriptive word to set your writing apart from others.
Context: ...inding generation script robustness and get it to work with a forked Cosmos-SDK dependenc...
(GET_STYLISH)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: lint
- GitHub Check: integration-tests
- GitHub Check: proto-gen-rs
- GitHub Check: e2e-evm
- GitHub Check: unit-tests
- GitHub Check: build
Unique-Divine
changed the title
…rFrom to transfer This change simplifies the `ConvertEvmToCoin` flow for ERC20-originated tokens by replacing the use of `transferFrom` with `transfer`. As a result, token approvals are no longer needed prior to conversion.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
x/evm/keeper/msg_server.go (1)
823-824: Use the actual sender address for burn operation.The EVM module address should not be the sender for
burnFromAuthoritycall on coin-originated tokens. The actual sender should be used as theFromaddress to properly represent who is initiating the burn operation.Apply this diff to fix the sender address:
- From: evm.EVM_MODULE_ADDRESS, - Nonce: k.GetAccNonce(ctx, evm.EVM_MODULE_ADDRESS), + From: senderEthAddr, + Nonce: k.GetAccNonce(ctx, senderEthAddr),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)x/evm/keeper/funtoken_evm_to_coin_test.go(1 hunks)x/evm/keeper/msg_server.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/keeper/funtoken_evm_to_coin_test.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2315
File: app/upgrades/v2_5_0/v2_5_0.go:0-0
Timestamp: 2025-05-21T21:47:04.451Z
Learning: In the v2.5.0 upgrade handler for Nibiru Chain, excess stNIBI ERC20 token balances (those not belonging to explicitly tracked holders) are sent to a Nibiru team-controlled Gnosis Safe for redistribution to ensure no tokens are lost during contract upgrades.
x/evm/keeper/msg_server.go (12)
Learnt from: Unique-Divine
PR: #2095
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-10-31T18:28:26.466Z
Learning: When implementing the bankkeeper.Keeper interface in x/evm/keeper/bank_extension.go, methods must use non-pointer receivers to satisfy the interface requirements.
Learnt from: Unique-Divine
PR: #2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In x/evm/keeper/bank_extension.go, assigning a new GasMeter to the context within the NibiruBankKeeper.ForceGasInvariant function is intentional and acceptable. This ensures that subsequent operations do not affect the parent ctx's gas consumption.
Learnt from: Unique-Divine
PR: #2119
File: x/evm/keeper/bank_extension_test.go:161-165
Timestamp: 2024-12-06T16:09:01.767Z
Learning: In Go test assertions within x/evm/keeper/bank_extension_test.go, when comparing uint64 values using s.Equalf, prefer formatting the values as decimal strings with fmt.Sprintf("%d", value) to avoid them being printed in hexadecimal.
Learnt from: Unique-Divine
PR: #2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the ApplyEvmTx function within x/evm/keeper/msg_server.go, a zero value for weiPerGas is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.
Learnt from: Unique-Divine
PR: #2097
File: x/oracle/keeper/keeper.go:195-195
Timestamp: 2024-10-31T09:55:25.630Z
Learning: In x/oracle/keeper/keeper.go, within the GetDatedExchangeRate method, it's not possible for exchangeRate.CreatedBlock to be in the future relative to ctx.BlockHeight(), so additional validation before time calculation is unnecessary.
Learnt from: Unique-Divine
PR: #1687
File: x/perp/v2/keeper/amm.go:0-0
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The user has addressed the issue of explicitly handling the case where costAmt is zero in the function handleMarketUpdateCost in the file x/perp/v2/keeper/amm.go.
Learnt from: k-yang
PR: #2093
File: x/evm/precompile/funtoken.go:85-87
Timestamp: 2024-11-04T20:38:45.909Z
Learning: In the Run methods of precompiled contracts in x/evm/precompile/, after handling errors from OnRunStart, it's unnecessary to check if err is nil before consuming gas with contract.UseGas(), as err will always be nil at that point.
Learnt from: onikonychev
PR: #2150
File: x/evm/embeds/contracts/TestERC20MaliciousTransfer.sol:36-38
Timestamp: 2025-01-31T07:16:54.084Z
Learning: Mint functions in ERC20 tokens should have access control (e.g., using OpenZeppelin's Ownable) to prevent unauthorized minting. The function should check for owner privileges using require(msg.sender == owner(), "must be owner").
Learnt from: Unique-Divine
PR: #1680
File: x/perp/v2/keeper/msg_server.go:198-198
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The user Unique-Divine has indicated that the error check for sdk.AccAddressFromBech32(msg.Sender) is not necessary in the context of their code because it is covered by the ValidateBasic method.
Learnt from: Unique-Divine
PR: #2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in x/evm/keeper/bank_extension_test.go, prefer comparing gas consumption values as formatted strings using fmt.Sprintf("%d", value) instead of numeric values directly in s.Equalf. This avoids hexadecimal representations that are hard to read in shell outputs.
Learnt from: Unique-Divine
PR: #1680
File: x/perp/v2/keeper/msg_server.go:210-210
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The user indicated that error handling for sdk.AccAddressFromBech32(msg.Sender) is covered by prior ValidateBasic checks in the code flow.
Learnt from: Unique-Divine
PR: #1680
File: x/perp/v2/keeper/msg_server.go:210-210
Timestamp: 2024-07-27T01:11:54.422Z
Learning: The user indicated that error handling for sdk.AccAddressFromBech32(msg.Sender) is covered by prior ValidateBasic checks in the code flow.
CHANGELOG.md (2)
Learnt from: Unique-Divine
PR: #2315
File: app/upgrades/v2_5_0/v2_5_0.go:0-0
Timestamp: 2025-05-21T21:47:04.451Z
Learning: In the v2.5.0 upgrade handler for Nibiru Chain, excess stNIBI ERC20 token balances (those not belonging to explicitly tracked holders) are sent to a Nibiru team-controlled Gnosis Safe for redistribution to ensure no tokens are lost during contract upgrades.
Learnt from: CalicoNino
PR: #2317
File: token-registry/official_erc20s.json:31-31
Timestamp: 2025-05-30T17:11:36.101Z
Learning: In the Nibiru token registry, the USDC (Stargate) token intentionally uses different symbols in different files: "USDC.e" in token-registry/official_erc20s.json and "USDC" in token-registry/cosmos_assetlist.go. This difference is deliberate and should not be flagged as an inconsistency.
🪛 LanguageTool
CHANGELOG.md
[style] ~55-~55: Replace ‘get’ with a more descriptive word to set your writing apart from others.
Context: ...inding generation script robustness and get it to work with a forked Cosmos-SDK dependenc...
(GET_STYLISH)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: e2e-evm
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: integration-tests
- GitHub Check: proto-gen-rs
- GitHub Check: build
🔇 Additional comments (2)
x/evm/keeper/msg_server.go (2)
756-792: LGTM! Well-structured delegation pattern.The main
ConvertEvmToCoinmethod properly validates the FunToken mapping and delegates to appropriate handlers based on token origin. The implementation follows established patterns in the codebase.
904-944: Verify the transfer approach for ERC20-originated tokens.The current implementation uses
transferwith the sender's Ethereum address asfromAccinCallContractWithInput. This approach avoids the approval mechanism complexity mentioned in past reviews. However, ensure this correctly represents the user transferring their tokens to the EVM module.The implementation looks correct as it uses:
senderEthAddras thefromAccparameter, representing the actual token holder- Direct
transfercall to move tokens from sender to EVM module- Proper error handling and state management
There was a problem hiding this comment.
- Handle the case of WNIBI in
ConvertEvmToCoin - Make
ToAddrwork the same way is in theIFuntoken.sendToEvmmethod,
where the address can be Bech32 or an Eth address. - Remove need for approvals by setting the msg sender as the
ERC20.transfersender. - There's a bug in the amount of coins transferred in
convertEvmToCoinForERC20Originatedbecause the amount of tokens transferred inERC20.transferis not the same as the input amount.
Edit: All changes addressed
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2315
File: app/upgrades/v2_5_0/v2_5_0.go:0-0
Timestamp: 2025-05-21T21:47:04.451Z
Learning: In the v2.5.0 upgrade handler for Nibiru Chain, excess stNIBI ERC20 token balances (those not belonging to explicitly tracked holders) are sent to a Nibiru team-controlled Gnosis Safe for redistribution to ensure no tokens are lost during contract upgrades.
CHANGELOG.md (2)
Learnt from: Unique-Divine
PR: #2315
File: app/upgrades/v2_5_0/v2_5_0.go:0-0
Timestamp: 2025-05-21T21:47:04.451Z
Learning: In the v2.5.0 upgrade handler for Nibiru Chain, excess stNIBI ERC20 token balances (those not belonging to explicitly tracked holders) are sent to a Nibiru team-controlled Gnosis Safe for redistribution to ensure no tokens are lost during contract upgrades.
Learnt from: CalicoNino
PR: #2317
File: token-registry/official_erc20s.json:31-31
Timestamp: 2025-05-30T17:11:36.101Z
Learning: In the Nibiru token registry, the USDC (Stargate) token intentionally uses different symbols in different files: "USDC.e" in token-registry/official_erc20s.json and "USDC" in token-registry/cosmos_assetlist.go. This difference is deliberate and should not be flagged as an inconsistency.
🪛 LanguageTool
CHANGELOG.md
[style] ~55-~55: Replace ‘get’ with a more descriptive word to set your writing apart from others.
Context: ...inding generation script robustness and get it to work with a forked Cosmos-SDK dependenc...
(GET_STYLISH)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: proto-gen-rs
- GitHub Check: build
- GitHub Check: unit-tests
- GitHub Check: e2e-evm
- GitHub Check: lint
- GitHub Check: integration-tests
🔇 Additional comments (3)
CHANGELOG.md (3)
42-47: Unreleased block formatting looks good
Whitespace/comment tweak restores compliance with Keep a Changelog rules.
51-53: Accurate changelog entry for PR #2345
Correct message name and working GitHub URL.
60-61: Extra blank line before “Dependencies” is fine
Improves visual separation of sections.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
CHANGELOG.md (1)
62-62: Broken PR link fixed — thanksThis addresses the earlier 23344→2344 typo.
🧹 Nitpick comments (3)
CHANGELOG.md (3)
59-59: Grammar/clarity nitTighten the sentence and avoid “new version for …”.
Apply:
- - [#2334](https://github.com/NibiruChain/nibiru/pull/2334) - feat(evm-embeds): Publish new version for `@nibiruchain/[email protected]`, which updates `NibiruOracleChainLinkLike.sol` to have additional methods used by Aave. + - [#2334](https://github.com/NibiruChain/nibiru/pull/2334) - feat(evm-embeds): Publish `@nibiruchain/[email protected]`, updating `NibiruOracleChainLinkLike.sol` with additional methods used by Aave.
60-60: Wrap message name in backticks for consistency with code identifiersOther code-like terms use backticks; do the same here.
- - [#2345](https://github.com/NibiruChain/nibiru/pull/2345) - feat(evm): add "eth.evm.v1.MsgConvertEvmToCoin" tx for ERC20 to bank coin conversions with a non-Ethereum transaction + - [#2345](https://github.com/NibiruChain/nibiru/pull/2345) - feat(evm): add `eth.evm.v1.MsgConvertEvmToCoin` tx for ERC20 to bank coin conversions with a non-Ethereum transaction
63-63: Wording nit: replace “get it to work”Reads more polished in release notes.
- - [#2346](https://github.com/NibiruChain/nibiru/pull/2346) - fix(buf-gen-rs): improve Rust proto binding generation script robustness and get it to work with a forked Cosmos-SDK dependency and exit correctly on failure + - [#2346](https://github.com/NibiruChain/nibiru/pull/2346) - fix(buf-gen-rs): improve Rust proto binding generation script robustness and make it work with a forked Cosmos-SDK dependency and exit correctly on failure
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
CHANGELOG.md(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-21T21:47:04.451Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2315
File: app/upgrades/v2_5_0/v2_5_0.go:0-0
Timestamp: 2025-05-21T21:47:04.451Z
Learning: In the v2.5.0 upgrade handler for Nibiru Chain, excess stNIBI ERC20 token balances (those not belonging to explicitly tracked holders) are sent to a Nibiru team-controlled Gnosis Safe for redistribution to ensure no tokens are lost during contract upgrades.
Applied to files:
CHANGELOG.md
🪛 LanguageTool
CHANGELOG.md
[grammar] ~59-~59: There might be a mistake here.
Context: ...to have additional methods used by Aave. - [#2345](https://github.com/NibiruChain/ni...
(QB_NEW_EN)
[style] ~63-~63: Replace ‘get’ with a more descriptive word to set your writing apart from others.
Context: ...inding generation script robustness and get it to work with a forked Cosmos-SDK dependenc...
(GET_STYLISH)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: integration-tests
- GitHub Check: proto-gen-rs
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: lint
- GitHub Check: e2e-evm
🔇 Additional comments (3)
CHANGELOG.md (3)
42-42: Blank line after Unreleased — LGTMImproves readability and keeps sections separated.
43-47: Fixed HTML comment opener spacing — LGTMRemoving the stray space before the comment tag is correct and keeps CI happy.
71-71: Blank line before Dependencies — LGTMMatches surrounding sections and improves scannability.
Unique-Divine
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
x/evm/errors.go (1)
13-24: Add a code for the new error.All module errors use registered codes. Introduce a code for ErrCanonicalWnibi to keep parity with the rest.
const ( codeErrInvalidState = uint32(iota) + 2 // NOTE: code 1 is reserved for internal errors codeErrZeroAddress codeErrInvalidAmount codeErrInvalidGasPrice codeErrInvalidGasFee codeErrInvalidRefund codeErrInvalidGasCap codeErrInvalidBaseFee codeErrInvalidAccount codeErrInactivePrecompile + codeErrCanonicalWnibi )app/upgrades/v2_5_0/v2_5_0.go (2)
185-191: Nil/contract check needed before dereferencing newErc20AccGuard against unexpected deployment failures to avoid panics when reading CodeHash.
Apply this diff:
newErc20Acc := keepers.EvmKeeper.GetAccount(ctx, newErc20Addr) +if newErc20Acc == nil || !newErc20Acc.IsContract() { + return fmt.Errorf("new ERC20 account not found or not a contract at %s", newErc20Addr.Hex()) +} originalErc20Account.CodeHash = newErc20Acc.CodeHash err = keepers.EvmKeeper.SetAccount(ctx, originalErc20Addr, *originalErc20Account) if err != nil { return fmt.Errorf("overwrite of contract bytecode failed: %w", err) }
200-213: Remove stale storage entries after copying new contract state
The upgrade only copies new slots under theoriginalErc20Addrprefix but never purges keys that existed on the old contract and are absent in the new one, which can lead to orphaned allowances or metadata corrupting ERC20 behavior. After the copy loop, track the new keys and then iterate over the original prefix to delete any key not in that set.Example diff:
iter := accState.Iterate(ctx, collections.PairRange[gethcommon.Address, gethcommon.Hash]{}.Prefix(newErc20Addr)) defer iter.Close() + newKeys := map[gethcommon.Hash]struct{}{} for ; iter.Valid(); iter.Next() { k2 := iter.Key().K2() + newKeys[k2] = struct{}{} accState.Insert(ctx, collections.Join(originalErc20Addr, k2), iter.Value()) } + // Remove stale keys on original + oldIter := accState.Iterate(ctx, collections.PairRange[gethcommon.Address, gethcommon.Hash]{}.Prefix(originalErc20Addr)) + defer oldIter.Close() + for ; oldIter.Valid(); oldIter.Next() { + k2 := oldIter.Key().K2() + if _, ok := newKeys[k2]; !ok { + _ = accState.Delete(ctx, collections.Join(originalErc20Addr, k2)) + } + }x/evm/keeper/msg_server.go (1)
635-657: Remove redundant StateDB commit
Inx/evm/keeper/msg_server.gothe callevmResp, err := k.CallContractWithInput(..., true /*commit*/, ...)already commits
stateDBviaApplyEvmMsg. Delete the subsequent explicit commit:- if err = stateDB.Commit(); err != nil { - return nil, sdkioerrors.Wrap(err, "failed to commit stateDB") - }
♻️ Duplicate comments (1)
x/evm/keeper/msg_convert_evm_to_coin.go (1)
23-26: Fix typo in function comment.The comment incorrectly references "MsgConvertCoinToEvm" when it should be "MsgConvertEvmToCoin".
// convertEvmToCoinForCoinOriginated is part of the -// "eth.evm.v1.MsgConvertCoinToEvm" tx. This function handles conversion of ERC20 +// "eth.evm.v1.MsgConvertEvmToCoin" tx. This function handles conversion of ERC20 // tokens that were originally bank coins back into coin form. The EVM module
🧹 Nitpick comments (43)
x/evm/vmtracer.go (1)
31-33: Disable debug by default — LGTM; consider a runtime override.Looks good to quiet logs. Consider wiring an env/config flag to flip this in tests or local dev without code changes.
Also applies to: 58-60
CHANGELOG.md (1)
64-65: Consistency and tone nits.
- “evm” → “EVM”.
- Replace “get it to work” with “make it work”.
- - [#2344](https://github.com/NibiruChain/nibiru/pull/2344) - feat(evm): Add some evm messages into the evm codec. + - [#2344](https://github.com/NibiruChain/nibiru/pull/2344) - feat(evm): Add some EVM messages into the EVM codec. - - [#2346](https://github.com/NibiruChain/nibiru/pull/2346) - fix(buf-gen-rs): improve Rust proto binding generation script robustness and get it to work with a forked Cosmos-SDK dependency and exit correctly on failure + - [#2346](https://github.com/NibiruChain/nibiru/pull/2346) - fix(buf-gen-rs): improve Rust proto binding generation script robustness and make it work with a forked Cosmos-SDK dependency; exit correctly on failurex/evm/evmtest/test_deps.go (1)
90-180: WNIBI deployment helper: tighten state moves and assertions.Solid approach. Two follow-ups:
- After copying storage from tempWnibiAddr to wnibiAddr, consider removing the temp prefix to avoid stale state that could interfere with reads/tests.
- Optionally assert the expected deploy address to catch nonce drift.
@@ evmResp, err := deps.EvmKeeper.CallContractWithInput( @@ } + // Optional: sanity-check expected deploy address + // if evmResp.ContractAddr != tempWnibiAddr { return fmt.Errorf("unexpected deploy addr: got %s want %s", evmResp.ContractAddr.Hex(), tempWnibiAddr.Hex()) } @@ { iter := evmAccState.Iterate(ctx, collections.PairRange[gethcommon.Address, gethcommon.Hash]{}.Prefix(tempWnibiAddr)) defer iter.Close() for ; iter.Valid(); iter.Next() { evmAccState.Insert( ctx, collections.Join(wnibiAddr, iter.Key().K2()), iter.Value(), ) + // Optional: clean up temp storage to prevent stale state in tests. + // evmAccState.Remove(ctx, collections.Join(tempWnibiAddr, iter.Key().K2())) } }If Bank.StateDB indirection here is relied on for journaling, please confirm no parallel tests touch it; otherwise, consider passing a fresh StateDB instead of mutating the keeper field.
api/eth/evm/v1/tx.pulsar.go (2)
9476-9573: API naming symmetry: consider aligning “to_addr” with existing “to_eth_addr”.Today: Coin→EVM uses to_eth_addr; EVM→Coin uses to_addr (dual-format). For discoverability, consider either:
- keep to_addr but document clearly in tx.proto that both hex and bech32 are accepted, or
- use a oneof { to_eth_addr | to_bech32_addr } to make intent explicit.
Proto change only; regenerate this file afterwards.
9496-9498: Clarify wasm-address caveat relative to this message.The caveat mentions wasm contracts cannot hold ERC20 (true), but this Tx mints bank coins to to_addr. If to_addr is a wasm bech32 address, that should be safe. Consider rewording in tx.proto to avoid implying this Tx is unsafe with wasm addresses; the asymmetry matters when target is ERC20.
x/evm/precompile/test/export.go (1)
200-201: CallContractWithInput call-sites updated for new argTests pass nil for weiValue consistently; good for queries/execs that don’t use msg.value.
If msg.value ever matters for Wasm precompile, consider adding one test that passes a non-zero value to validate forwarding semantics.
Also applies to: 305-306
x/evm/keeper/erc20.go (3)
79-80: Consistent zero-value ERC20 calls; consider a small helper to reduce repetitionAll ERC20 Mint/Transfer/Burn and loaders now pass nil weiValue (i.e., 0), which is correct. To avoid footguns and repeated literals, consider thin helpers for “no-value” calls.
Example (outside this hunk) to centralize the pattern:
// in keeper func (k *Keeper) callNoValue(ctx sdk.Context, evmObj *vm.EVM, from gethcommon.Address, to *gethcommon.Address, commit bool, input []byte, gas uint64) (*evm.MsgEthereumTxResponse, error) { return k.CallContractWithInput(ctx, evmObj, from, to, commit, input, gas, nil) }Then here:
return e.callNoValue(ctx, evmObj, sender, &erc20Contract, false, contractInput, getCallGasWithLimit(ctx, Erc20GasLimitExecute))Also applies to: 105-106, 177-178, 217-218, 261-262, 305-306
33-39: Doc nit: function name mismatchComment says “getCallGas returns…” but the function is getCallGasWithLimit. Align the doc to the actual name.
-// getCallGas returns the gas limit for a call to an ERC20 contract following 63/64 rule (EIP-150) +// getCallGasWithLimit returns the gas limit for a call to an ERC20 contract following the 63/64 rule (EIP-150)
152-159: Typo in comment“amaount” → “amount”.
-// TotalSupply returns the amaount of ERC20 tokens in existence. +// TotalSupply returns the amount of ERC20 tokens in existence.app/upgrades/v2_5_0/v2_5_0.go (1)
183-191: Log code hashes as hex, not raw bytesCurrent string() prints unreadable/binary data. Prefer hex for diagnostics.
-fmt.Println("old code hash", string(originalErc20Account.CodeHash)) +fmt.Printf("old code hash %x\n", originalErc20Account.CodeHash) ... -fmt.Println("new code hash", string(newErc20Acc.CodeHash)) +fmt.Printf("new code hash %x\n", newErc20Acc.CodeHash)x/evm/keeper/call_contract.go (1)
30-31: Docstring is stale about gas limit.It says “uses a default gas limit” but the function takes gasLimit.
-// Note: This function handles both contract method calls and simulations, -// depending on the 'commit' parameter. It uses a default gas limit. +// Note: This function handles both contract method calls and simulations, +// depending on the 'commit' parameter. The provided gasLimit is used as-is.x/evm/keeper/msg_convert_coin_to_evm.go (7)
25-31: Fix error prefix for insufficient funds.Wrong flow name in the message.
- err = fmt.Errorf( - "ConvertEvmToCoin: insufficient funds to send WNIBI, balance %s, msg.BankCoin %s", balMicronibi, msg.BankCoin, - ) + err = fmt.Errorf("ConvertCoinToEvm: insufficient funds to send WNIBI, balance %s, msg.BankCoin %s", + balMicronibi, msg.BankCoin)
72-73: Typo in error message (“despoit”).- err = fmt.Errorf("ABI packing error in WNIBI.despoit: %w", err) + err = fmt.Errorf("ABI packing error in WNIBI.deposit: %w", err)
98-103: Fix error prefix when querying balance.- err = fmt.Errorf("ConvertEvmToCoin: failed to query ERC20 balance: %w", err) + err = fmt.Errorf("ConvertCoinToEvm: failed to query ERC20 balance: %w", err)
114-120: Correct direction in error messages for deposit.It’s a NIBI→WNIBI deposit.
- return resp, fmt.Errorf("failed to convert WNIBI to NIBI: %w", err) + return resp, fmt.Errorf("WNIBI deposit failed: %w", err) @@ - err = fmt.Errorf("failed to convert WNIBI to NIBI: VmError: %s", evmResp.VmError) + err = fmt.Errorf("WNIBI deposit failed: VmError: %s", evmResp.VmError)
121-126: Fix error prefix when querying post-deposit balance.- err = fmt.Errorf("ConvertEvmToCoin: failed to query ERC20 balance: %w", err) + err = fmt.Errorf("ConvertCoinToEvm: failed to query ERC20 balance: %w", err)
132-135: Comment typo (“tranfers”).-// STEP 2: Sender tranfers WNIBI to intended recipient +// STEP 2: Sender transfers WNIBI to intended recipient
143-147: Correct direction and typos in transfer errors.- return resp, fmt.Errorf("failed to convert WNIBI to NIBI: %w", err) + return resp, fmt.Errorf("WNIBI transfer failed: %w", err) @@ - err = fmt.Errorf("failed to convert WNIBI to NIBI: VmError: %s", evmResp.VmError) + err = fmt.Errorf("WNIBI transfer failed: VmError: %s", evmResp.VmError) @@ - "ConvertCoinToEvm: transfer of WNIBI succeeded but did not deliver the correct number of tokens: transfer amount %s, balance increase %s, senderHex %s, receipient %s", + "ConvertCoinToEvm: transfer of WNIBI succeeded but did not deliver the correct number of tokens: transfer amount %s, balance increase %s, senderHex %s, recipient %s",Also applies to: 150-155
x/evm/evmtest/erc20.go (3)
124-131: Nice helper type; consider documenting zero/absent ERC20 case.
Document that BalanceERC20=nil skips the ERC20 assertion; if non-nil and CanonicalWnibi is unset, the helper will fail on BalanceOf. Clarify expected behavior in such networks.
132-151: Handle EVM creation errors in tests.
deps.NewEVM() error is ignored; assert it to avoid silent failures.Apply this diff:
- } else { - evmObj, _ = deps.NewEVM() + } else { + var err error + evmObj, err = deps.NewEVM() + assert.NoError(t, err, "NewEVM failed") }
145-151: Prefer direct address comparison.
When checking CanonicalWnibi, compare addresses rather than hex strings to avoid case/encoding pitfalls.Example:
erc20 := deps.EvmKeeper.GetParams(deps.Ctx).CanonicalWnibi.Addressx/evm/keeper/msg_convert_coin_to_evm_test.go (3)
66-88: Fragile assertions on EventTxLog dynamic fields.
Hard-coding BlockNumber/TxHash/Index can flake as internals evolve. Prefer asserting address, topics, and data only.Example approach: fetch evm.EventTxLog from ctx and check first log fields without asserting block/tx metadata.
293-331: Gas comparison is brittle.
Requiring SDK gas > EVM gas could fail with benign changes. Assert both are non-zero and that SDK gas >= EVM gas, or just non-zero.Apply this diff:
- s.Require().Greaterf(deps.Ctx.GasMeter().GasConsumed(), evmResp.GasUsed, "total gas consumed on cosmos context should be greater than gas used by EVM") + s.Require().GreaterOrEqualf(deps.Ctx.GasMeter().GasConsumed(), evmResp.GasUsed, "cosmos gas should be >= EVM gas")
743-764: Misleading error prefix for WNIBI coin→EVM path.
The expected error contains “ConvertEvmToCoin…”, but the code path is ConvertCoinToEvmForWNIBI. Standardize the message (recommend “ConvertCoinToEvm: insufficient funds to send WNIBI”) or relax the test to assert only the “insufficient funds to send WNIBI” substring.Apply this diff to the test for robustness:
- s.Require().ErrorContains(err, "ConvertEvmToCoin: insufficient funds to send WNIBI") + s.Require().ErrorContains(err, "insufficient funds to send WNIBI")x/evm/keeper/msg_server.go (2)
779-825: ConvertEvmToCoin: solid routing; prefer address equality over Hex().
Compare geth addresses directly to avoid checksum/case issues.Apply this diff:
- if erc20.Hex() == evmParams.CanonicalWnibi.Hex() { + if erc20.Address == evmParams.CanonicalWnibi.Address {
807-813: Improve mapping error message.
Return count to aid diagnostics when duplicates exist.- err = fmt.Errorf("no FunToken mapping exists for ERC20 \"%s\"", erc20.Hex()) + err = fmt.Errorf("expected 1 FunToken mapping for ERC20 %s, found %d", erc20.Hex(), len(funTokens))x/evm/keeper/funtoken_from_coin_test.go (4)
81-90: Potential nonce-derived address flakiness.You pre-compute expectedErc20Addr from the module account nonce before CreateFunToken. If any prior EVM tx from the module increments the nonce, this assertion could flake. Consider scoping this test to guarantee no earlier module EVM tx or derive/compare the deployed address only after CreateFunToken.
140-168: Prefer typed event assertion over manual topic hashing.Manual EventTxLog topic construction is brittle. Use the existing typed event helper for stability.
Apply:
- testutil.RequireContainsTypedEvent( - s.T(), - deps.Ctx, - &evm.EventTxLog{ - Logs: []evm.Log{ ... }, - }, - ) +testutil.RequireContainsTypedEvent( + s.T(), + deps.Ctx, + &evm.EventTxLog{}, // presence is sufficient; decode and assert fields selectively if needed +)
197-208: Fix units in comments for clarity.The narrative mixes “1” vs “1e6”. Use explicit magnitudes to avoid confusion.
-// - Test contract funds: 0 EVM -// - Alice: 1 EVM, 9 BC -// - Module account: 1 BC escrowed (which Alice holds as 1 EVM) +// - Test contract funds: 0 ERC20 +// - Alice: 1,000,000 ERC20, 9,000,000 Bank +// - Module account: 1,000,000 Bank escrowed (Alice holds 1,000,000 as ERC20)
270-279: New CallContractWithInput arg.Passing nil for the new trailing param is consistent with the updated signature. Consider a brief comment indicating it’s the optional value/override for readability.
x/evm/precompile/funtoken_test.go (3)
83-98: Use test logger instead of fmt.Printf.Avoid stdout noise; prefer t.Logf.
- fmt.Printf("arg: %s", arg) + t.Logf("arg: %s", arg)
443-453: Minor: expand subtest name.Clarify the recipient context in the title for quick scanning in CI logs.
-s.Run("Sending 400 tokens back from EVM to Cosmos bank => recipient:", func() { +s.Run("sendToBank: send 400 from EVM to random Cosmos recipient", func() {
571-575: Use bigTokens helper for 18-dec amounts.Consistent units improve readability in ERC20-originated tests.
- big.NewInt(1000) + bigTokens(1000)x/evm/keeper/msg_convert_evm_to_coin_test.go (3)
185-229: ERC20-originated: unit consistency suggestion.convertAmount is raw units (not human 18-dec). Consider using bigTokens helper to make intent obvious, or add a comment that amounts are in raw ERC20 units.
- convertAmount := sdkmath.NewInt(100000) + // Amounts are in raw ERC20 units (18 decimals). 100k raw = 1e5. + convertAmount := sdkmath.NewInt(100_000) // raw units
231-290: Subtest naming mismatch.Title says “no approval to transfer” (sad path) but the flow intends success because the call origin is the sender. Rename for clarity.
-s.Run("sad: no approval to transfer", func() { +s.Run("no ERC20 approve required: call originates from sender", func() {
356-378: Use typed event assertion instead of scanning attributes-as-strings.Manual string/quote handling is brittle. Prefer testutil.RequireContainsTypedEvent with the generated event type and then assert fields.
- foundEvent := false - for _, event := range deps.Ctx.EventManager().Events() { - if event.Type == "eth.evm.v1.EventConvertEvmToCoin" { - foundEvent = true - ... - } - } - s.Require().True(foundEvent, "EventConvertEvmToCoin not found") + testutil.RequireContainsTypedEvent( + s.T(), + deps.Ctx, + &evm.EventConvertEvmToCoin{ + Sender: deps.Sender.NibiruAddr.String(), + Erc20ContractAddress: erc20Addr.String(), + ToAddress: toAddr.String(), + // BankCoin: check denom and amount only if needed + }, + )x/evm/msg.go (1)
513-520: Improved error message clarity.The updated error messages now include a descriptive prefix "error in MsgConvertCoinToEvm" which helps with debugging and log analysis.
For consistency, consider using the same error wrapping pattern across all validation methods:
- return fmt.Errorf("error in MsgConvertCoinToEvm: invalid sender addr: %w", err) + return sdkioerrors.Wrapf(err, "error in MsgConvertCoinToEvm: invalid sender addr")x/evm/keeper/msg_convert_evm_to_coin.go (1)
290-291: Consider using more descriptive variable names in error message.The error message could be clearer with formatted values showing the units.
- err = fmt.Errorf("WNIBI withdraw failed: withdraw amount %s, balBefore %s, balAfter %s", withdrawWei, wnibiBalBefore, wnibiBalAfter) + err = fmt.Errorf("WNIBI withdraw failed: expected withdraw %s wei, balance before %s wei, balance after %s wei", withdrawWei, wnibiBalBefore, wnibiBalAfter)x/evm/evmmodule/genesis_test.go (5)
59-61: Don't ignore NewEVM error; fail fast in tests.Capture and assert the error to avoid silent misconfigurations.
-evmObj, _ := deps.NewEVM() +evmObj, err := deps.NewEVM() +s.Require().NoError(err)
125-131: *Typo and redundant &; pass the correct value directly.Rename bankGensisState → bankGenesisState and drop the no-op *&.
-authGenesisState := deps.App.AccountKeeper.ExportGenesis(deps.Ctx) -bankGensisState := deps.App.BankKeeper.ExportGenesis(deps.Ctx) +authGenesisState := deps.App.AccountKeeper.ExportGenesis(deps.Ctx) +bankGenesisState := deps.App.BankKeeper.ExportGenesis(deps.Ctx) @@ deps = evmtest.NewTestDeps() deps.App.AccountKeeper.InitGenesis(deps.Ctx, *authGenesisState) -deps.App.BankKeeper.InitGenesis(deps.Ctx, *&bankGensisState) +deps.App.BankKeeper.InitGenesis(deps.Ctx, bankGenesisState)
74-87: Fix typos in log and comment.
- “er20” → “ERC20”
- Duplicate “needs” in comment.
-s.T().Logf("Create FunToken from the er20 %s", erc20Addr) +s.T().Logf("Create FunToken from the ERC20 %s", erc20Addr) @@ -// There's a fee for "evm.MsgCreateFunToken". The sender account needs -// needs funds for that. +// There's a fee for "evm.MsgCreateFunToken". The sender account needs funds for that.
142-147: Avoid int64 truncation; use big.Int arithmetic for expectations.Using Int64() then adding can overflow/truncate for large amounts.
-s.Require().Equal( - new(big.Int).Sub(totalSupply, big.NewInt(amountToSendA.Int64()+amountToSendB.Int64())), - balance, -) +sumAB := new(big.Int).Add(new(big.Int).Set(amountToSendA), amountToSendB) +expected := new(big.Int).Sub(new(big.Int).Set(totalSupply), sumAB) +s.Require().Equal(expected, balance)
154-155: Normalize address comparison to a single representation.Use the same formatter on both sides to avoid case/checksum mismatches.
-s.Equal(ft.Erc20Addr.Hex(), funTokens[0].Erc20Addr.String()) +s.Equal(ft.Erc20Addr.String(), funTokens[0].Erc20Addr.String())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/evm/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (31)
CHANGELOG.md(2 hunks)api/eth/evm/v1/tx.pulsar.go(6 hunks)app/appconst/appconst.go(2 hunks)app/upgrades.go(2 hunks)app/upgrades/v2_5_0/v2_5_0.go(3 hunks)app/upgrades/v2_5_0/v2_5_0_test.go(1 hunks)app/upgrades/v2_7_0/v2_7_0.go(1 hunks)eth/eip55.go(1 hunks)proto/eth/evm/v1/tx.proto(11 hunks)x/evm/errors.go(1 hunks)x/evm/evmmodule/genesis_test.go(5 hunks)x/evm/evmtest/erc20.go(3 hunks)x/evm/evmtest/test_deps.go(3 hunks)x/evm/keeper/call_contract.go(1 hunks)x/evm/keeper/erc20.go(7 hunks)x/evm/keeper/funtoken_from_coin.go(1 hunks)x/evm/keeper/funtoken_from_coin_test.go(6 hunks)x/evm/keeper/funtoken_from_erc20_test.go(15 hunks)x/evm/keeper/keeper_test.go(1 hunks)x/evm/keeper/msg_convert_coin_to_evm.go(1 hunks)x/evm/keeper/msg_convert_coin_to_evm_test.go(1 hunks)x/evm/keeper/msg_convert_evm_to_coin.go(1 hunks)x/evm/keeper/msg_convert_evm_to_coin_test.go(1 hunks)x/evm/keeper/msg_server.go(5 hunks)x/evm/msg.go(7 hunks)x/evm/precompile/funtoken_test.go(22 hunks)x/evm/precompile/oracle_test.go(2 hunks)x/evm/precompile/test/export.go(2 hunks)x/evm/precompile/wasm_test.go(10 hunks)x/evm/statedb/journal_test.go(2 hunks)x/evm/vmtracer.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- eth/eip55.go
- proto/eth/evm/v1/tx.proto
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2024-10-31T18:28:26.466Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2095
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-10-31T18:28:26.466Z
Learning: When implementing the `bankkeeper.Keeper` interface in `x/evm/keeper/bank_extension.go`, methods must use non-pointer receivers to satisfy the interface requirements.
Applied to files:
x/evm/keeper/msg_convert_coin_to_evm.gox/evm/keeper/msg_convert_evm_to_coin.go
📚 Learning: 2025-01-17T17:32:27.341Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns only `*vm.EVM` without an error value.
Applied to files:
x/evm/evmtest/test_deps.go
📚 Learning: 2025-01-17T17:32:33.412Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns both an EVM object and a StateDB object with the signature `(*vm.EVM, *statedb.StateDB)`. The method is used in test files to create new EVM and StateDB instances for testing purposes.
Applied to files:
x/evm/evmtest/test_deps.go
📚 Learning: 2025-01-17T17:32:27.341Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns `(*vm.EVM, *statedb.StateDB)` without an error value. The second return value is often discarded when only the EVM object is needed.
Applied to files:
x/evm/evmtest/test_deps.go
📚 Learning: 2025-01-17T17:32:33.412Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns only an EVM object and does not return an error. The method is used in test files to create a new EVM instance for testing purposes.
Applied to files:
x/evm/evmtest/test_deps.go
📚 Learning: 2024-12-06T11:54:38.275Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
Applied to files:
x/evm/evmtest/test_deps.gox/evm/precompile/funtoken_test.go
📚 Learning: 2024-12-06T16:09:01.767Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2119
File: x/evm/keeper/bank_extension_test.go:161-165
Timestamp: 2024-12-06T16:09:01.767Z
Learning: In Go test assertions within `x/evm/keeper/bank_extension_test.go`, when comparing `uint64` values using `s.Equalf`, prefer formatting the values as decimal strings with `fmt.Sprintf("%d", value)` to avoid them being printed in hexadecimal.
Applied to files:
x/evm/evmtest/test_deps.gox/evm/precompile/funtoken_test.gox/evm/evmmodule/genesis_test.gox/evm/keeper/funtoken_from_coin_test.go
📚 Learning: 2025-05-21T21:47:04.451Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2315
File: app/upgrades/v2_5_0/v2_5_0.go:0-0
Timestamp: 2025-05-21T21:47:04.451Z
Learning: In the v2.5.0 upgrade handler for Nibiru Chain, excess stNIBI ERC20 token balances (those not belonging to explicitly tracked holders) are sent to a Nibiru team-controlled Gnosis Safe for redistribution to ensure no tokens are lost during contract upgrades.
Applied to files:
app/upgrades/v2_5_0/v2_5_0.goCHANGELOG.md
📚 Learning: 2024-12-26T04:43:43.966Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2129
File: x/evm/embeds/contracts/TestInfiniteRecursionERC20.sol:24-32
Timestamp: 2024-12-26T04:43:43.966Z
Learning: The `TestInfiniteRecursionERC20` contract is intentionally designed to test infinite recursion scenarios in ERC20-based contracts for the `FunToken` architecture.
Applied to files:
x/evm/keeper/funtoken_from_erc20_test.go
📚 Learning: 2024-10-13T05:48:51.771Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the `ApplyEvmTx` function within `x/evm/keeper/msg_server.go`, a zero value for `weiPerGas` is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.
Applied to files:
x/evm/keeper/funtoken_from_erc20_test.gox/evm/precompile/funtoken_test.gox/evm/keeper/msg_server.go
📚 Learning: 2024-11-04T20:38:45.909Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2093
File: x/evm/precompile/funtoken.go:85-87
Timestamp: 2024-11-04T20:38:45.909Z
Learning: In the `Run` methods of precompiled contracts in `x/evm/precompile/`, after handling errors from `OnRunStart`, it's unnecessary to check if `err` is nil before consuming gas with `contract.UseGas()`, as `err` will always be nil at that point.
Applied to files:
x/evm/keeper/funtoken_from_erc20_test.gox/evm/precompile/funtoken_test.go
📚 Learning: 2024-10-08T15:45:59.488Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#1680
File: x/perp/v2/keeper/msg_server.go:210-210
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The user indicated that error handling for `sdk.AccAddressFromBech32(msg.Sender)` is covered by prior `ValidateBasic` checks in the code flow.
Applied to files:
x/evm/msg.go
📚 Learning: 2024-10-08T15:45:59.488Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#1680
File: x/perp/v2/keeper/msg_server.go:198-198
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The user Unique-Divine has indicated that the error check for `sdk.AccAddressFromBech32(msg.Sender)` is not necessary in the context of their code because it is covered by the `ValidateBasic` method.
Applied to files:
x/evm/msg.go
🧬 Code graph analysis (7)
app/upgrades/v2_7_0/v2_7_0.go (3)
app/upgrades/v2_6_0/v2_6_0.go (2)
Upgrade(11-15)UpgradeName(9-9)app/keepers/all_keepers.go (1)
PublicKeepers(35-75)eth/eip55.go (1)
EIP55Addr(17-19)
x/evm/keeper/funtoken_from_coin.go (2)
x/evm/const.go (1)
EVM_MODULE_ADDRESS(106-106)x/evm/keeper/erc20.go (1)
Erc20GasLimitDeploy(22-22)
x/evm/keeper/msg_convert_evm_to_coin_test.go (8)
eth/eip55.go (1)
EIP55Addr(17-19)x/evm/evmtest/eth.go (1)
NewEthPrivAcc(27-37)x/evm/evmtest/tx.go (2)
DeployContract(58-100)TxTransferWei(186-191)x/evm/embeds/embeds.go (2)
SmartContract_TestERC20(101-104)SmartContract_WNIBI(96-99)x/evm/const.go (2)
EVM_MODULE_ADDRESS(106-106)NativeToWei(123-126)x/evm/keeper/erc20.go (1)
Erc20GasLimitExecute(30-30)x/evm/params.go (1)
EVMBankDenom(25-25)x/common/testutil/testapp/testapp.go (1)
FundFeeCollector(183-192)
x/evm/precompile/funtoken_test.go (3)
x/common/testutil/testapp/testapp.go (1)
FundAccount(156-165)x/evm/const.go (1)
EVM_MODULE_ADDRESS(106-106)eth/eth_account.go (1)
NibiruAddrToEthAddr(19-21)
x/evm/keeper/funtoken_from_coin_test.go (3)
x/evm/tx.pb.go (3)
MsgConvertCoinToEvm(582-589)MsgConvertCoinToEvm(593-593)MsgConvertCoinToEvm(594-596)x/evm/evm.pb.go (3)
FunToken(32-41)FunToken(45-45)FunToken(46-48)x/common/testutil/testapp/testapp.go (1)
FundAccount(156-165)
x/evm/keeper/msg_server.go (4)
x/evm/keeper/keeper.go (1)
Keeper(25-52)x/evm/tx.pb.go (3)
MsgConvertEvmToCoin(675-693)MsgConvertEvmToCoin(697-697)MsgConvertEvmToCoin(698-700)eth/eth_account.go (1)
NibiruAddrToEthAddr(19-21)x/evm/statedb/statedb.go (1)
StateDB(45-85)
x/evm/msg.go (4)
x/evm/tx.pb.go (6)
MsgConvertEvmToCoin(675-693)MsgConvertEvmToCoin(697-697)MsgConvertEvmToCoin(698-700)MsgEthereumTx(40-51)MsgEthereumTx(55-55)MsgEthereumTx(56-58)eth/eip55.go (2)
EIP55Addr(17-19)NewEIP55AddrFromStr(22-35)eth/eth_account.go (2)
EthAddrToNibiruAddr(15-17)NibiruAddrToEthAddr(19-21)x/evm/codec.go (1)
AminoCdc(18-18)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~61-~61: There might be a mistake here.
Context: ...to have additional methods used by Aave. - [#2345](https://github.com/NibiruChain/ni...
(QB_NEW_EN)
[style] ~65-~65: Replace ‘get’ with a more descriptive word to set your writing apart from others.
Context: ...inding generation script robustness and get it to work with a forked Cosmos-SDK dependenc...
(GET_STYLISH)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: unit-tests
- GitHub Check: proto-gen-rs
- GitHub Check: integration-tests
- GitHub Check: build
- GitHub Check: lint
- GitHub Check: e2e-evm
🔇 Additional comments (64)
x/evm/precompile/wasm_test.go (1)
65-65: CallContractWithInput(nil) is safe
The keeper’s implementation checks for a nil weiValue and defaults tobig.NewInt(0), so passingnilcan’t cause a nil-dereference and correctly represents zero value.x/evm/evmtest/test_deps.go (1)
64-66: Less-verbose EVM logger — LGTM.api/eth/evm/v1/tx.pulsar.go (5)
8280-8634: Empty response type is fine; ensure events surface conversion outcomes.If clients rely on events to learn minted bank coins/recipient, confirm EventConvertEvmToCoin is emitted with sufficient details and tests assert it.
9729-9778: Registration and service wiring changes look consistent.
- Raw descriptor updated with the new RPC.
- NumMessages bumped to 14; goTypes/depIdxs entries align with indices 12/13 for the new messages.
Also applies to: 9802-9843
10001-10024: Exporter hooks and TypeBuilder counts updated correctly.No issues spotted.
Also applies to: 10032-10044
7668-7683: Generated file: avoid manual edits.If anything changes, update proto/eth/evm/v1/tx.proto and re-run codegen (buf/protoc) to keep this artifact stable.
7668-8278: Verify signer annotation and message validation
- Ensure the
MsgConvertEvmToCoinproto hasoption (cosmos.msg.v1.signer) = "sender"inside its message definition (not a global “authority”)- Confirm
MsgConvertEvmToCoin.Validate()(called byValidateBasic) enforces:
erc20_addris a 20-byte EVM address (EIP-55 or equivalent)amountis a positive, non-zero integerto_addronly allows permitted Cosmos/WASM mappings and rejects ambiguous contract addresses- Verify
Keeper.ConvertEvmToCoinrelies solely onmsg.Validate()for these checks, and no additional keeper‐level safeguards are requiredapp/upgrades/v2_5_0/v2_5_0_test.go (1)
182-183: API update acknowledged: passing nil weiValue for a view call is correctThe signature change is reflected properly here; nil maps to 0 value for a read-only query.
x/evm/keeper/funtoken_from_coin.go (1)
110-112: Zero-value deploy: explicit default looks fineUsing nil for weiValue keeps the deploy at 0-value, which matches the constructed evmMsg.Value. No issues.
app/upgrades.go (1)
33-34: Wired v2_7_0 upgrade in orderImport and append look correct. Ensure the on-chain schedule places v2_7_0 after v2_6_0 by height as expected.
Also applies to: 52-53
x/evm/precompile/oracle_test.go (2)
71-80: Updated CallContractWithInput signature: OKPassing nil for the new trailing arg keeps behavior unchanged here.
137-146: Second call site updated correctlyThe extra nil aligns with the new signature; no functional change to this test.
x/evm/keeper/keeper_test.go (2)
27-29: New SuiteFunToken type: OKProvides a clean receiver for FunToken tests used elsewhere.
18-21: Verified no duplicate TestAll entrypoints; other TestSuite definitions live in separate packages.x/evm/statedb/journal_test.go (2)
41-51: Adaptation to new CallContractWithInput param: OKnil for value is appropriate for this mint flow.
107-116: Adaptation to new CallContractWithInput param: OKConsistent with zero-wei call semantics.
app/upgrades/v2_5_0/v2_5_0.go (3)
20-20: Use centralized mainnet constantsImporting appconst reduces drift in hardcoded addresses.
44-45: Prefer appconst.MAINNET_STNIBI_ADDRGood replacement for the local var; fewer single-source-of-truth risks.
159-163: New CallContractWithInput arg: OKnil value is correct for a no-value CREATE.
app/appconst/appconst.go (2)
13-14: Importing gethcommon: OKRequired for typed addresses; keeps call sites clean.
31-38: Mainnet address constants: OKExported typed addresses are helpful. Nice docstrings.
x/evm/evmtest/erc20.go (3)
14-14: LGTM: appconst import fits usage.
164-169: LGTM: clearer balance labels.
155-159: All FunTokenBalanceAssert literals use named fields; no positional initializations found.x/evm/keeper/msg_convert_coin_to_evm_test.go (2)
398-399: No action needed:evmtest.DefaultEthCallGasLimitis defined inx/evm/evmtest/tx.goas an alias ofsrvconfig.DefaultEthCallGasLimit(it ultimately originates fromapp/server/config/server_config.go), so compilation will succeed.
769-801: Ignore type mismatch suggestion.ErrCanonicalWnibiis declared as a string constant, sos.Require().ErrorContains(err, evm.ErrCanonicalWnibi)is correct.Likely an incorrect or invalid review comment.
x/evm/keeper/msg_server.go (2)
551-564: Good: early validation and sender decoding.
559-563: No changes needed:convertCoinToEvmForWNIBIalready enforces “well-defined” WNIBI
The function immediately callsif stateDB.GetCodeSize(erc20.Address) == 0 { return fmt.Errorf(…) }which guarantees non-empty bytecode (and thus a valid, non-zero contract) before proceeding.
x/evm/keeper/funtoken_from_coin_test.go (4)
21-21: Suite rename is consistent with new test scaffolding.Receiver change to SuiteFunToken looks correct and aligns with the broader suite refactor.
36-50: Denom metadata setup LGTM.Accurate base unit and symbol; matches expected Decimals=0 later.
116-126: ERC20 metadata assertions are precise.Name/Symbol/Decimals checks match the configured bank metadata.
310-343: Helper’s funding logic is tidy and explicit.Combining fee + test funding reduces setup noise and improves test determinism.
x/evm/precompile/funtoken_test.go (10)
122-131: Bank funding tied to mapping denom is correct.Using funtoken.BankDenom avoids coupling to global EVMBankDenom.
156-174: Post-conversion balance checks are solid.ERC20 balance and bank escrow assertions match ConvertCoinToEvm semantics.
176-190: Negative mint path is precise.Expecting Ownable revert is appropriate for non-owner mint attempts.
219-224: sendToBank assertions: denom consistency.Good use of funtoken.BankDenom for bank-side checks.
263-266: CreateFunTokenForBankCoin: testdenomSwitch to a local test denom is fine; keeps tests self-contained.
299-316: Local gas default path test reads clearly.Commit=false in views and appropriate gas limits used.
321-341: Local gas sufficient path.Covers the precompile-local-gas branch without over-provisioning.
343-364: Local gas insufficient path.Asserting “execution reverted” is the right observable effect.
373-425: Made-from-coin sendToEvm path is thorough.Checks escrow on bank side and mint on ERC20 side; balances line up.
727-766: getErc20Address view: robust and clear.Happy/invalid/non-existent paths are exercised; using commit=false and explicit gas is good.
x/evm/keeper/msg_convert_evm_to_coin_test.go (11)
29-41: Bank metadata setup is correct.Minimal denom unit with symbol; aligns with Decimals=0 expectations for coin-originated mapping.
50-61: CreateFunToken from bank coin: OK.Capturing funtoken and erc20Addr for subsequent assertions is clear.
71-89: Bootstrap via ConvertCoinToEvm is well-structured.Pre-load sender’s ERC20 and verify escrowed bank zero on the account.
90-118: Happy path: EVM→Bank (coin-originated).Balance transitions and recipient bank credit are correct.
121-134: Insufficient ERC20 balance: good negative test.Amount intentionally exceeds remaining ERC20; expecting error is appropriate.
135-150: Non-existent mapping: error contract is asserted.Error text check is specific enough to catch regressions.
296-309: Event test: good metadata prep.Denom metadata and initial funding are set atomically; reduces fixture noise.
389-472: Multiple recipients: looped conversions are clear.Good coverage for sequential sends and final balance reconciliation.
477-499: WNIBI: precondition check reads well.Asserting failure when canonical address points to non-contract is valuable.
513-548: WNIBI wrapping: balance pre/post checks are precise.Funding fee collector and using TxTransferWei is realistic; verifying SDB and ERC20 balances is helpful.
550-575: WNIBI conversion happy path.Post-conversion checks on WNIBI and native balance are consistent with NativeToWei semantics.
x/evm/keeper/funtoken_from_erc20_test.go (2)
24-24: LGTM! Test suite refactoring is clean and consistent.The refactoring from
FunTokenFromErc20SuitetoSuiteFunTokenhas been applied consistently across all test methods. The changes follow the broader test suite reorganization pattern.Also applies to: 168-168, 362-362, 402-402, 461-461, 537-537, 629-629
215-215: Approve CallContractWithInput signature and call-site updates
AllCallContractWithInputcalls infuntoken_from_erc20_test.goinclude the newnilweiValue argument, and the keeper’s signature inx/evm/keeper/call_contract.gohas been updated to acceptweiValue *big.Int.x/evm/msg.go (5)
25-25: LGTM! Consistent usage of gethcommon alias throughout the file.The introduction of the
gethcommonalias forgithub.com/ethereum/go-ethereum/commonimproves code readability and is applied consistently across all relevant methods.Also applies to: 300-300, 322-322, 326-326, 409-409, 541-543, 555-556, 564-564, 567-568
529-533: LGTM! GetSigners implementation follows SDK standards.The implementation correctly extracts the sender address from the message and returns it as a single-element slice, consistent with other message types in the file.
536-578: Well-structured validation with comprehensive address handling.The
Validatemethod provides robust validation with support for both Ethereum hex and Bech32 address formats. The structured return type makes it convenient for callers to use the parsed values directly.
581-584: LGTM! ValidateBasic delegates to the comprehensive Validate method.The implementation correctly delegates to the
Validatemethod and discards the parsed values that are only needed by callers who want to use them.
586-589: LGTM! GetSignBytes implementation is consistent.The implementation follows the standard pattern used by other message types in the file.
x/evm/keeper/msg_convert_evm_to_coin.go (3)
27-111: Well-structured conversion logic for coin-originated tokens.The function correctly handles the conversion flow:
- Burns ERC20 tokens via the authority-controlled burn function
- Transfers equivalent bank coins from the module to the recipient
- Commits state and emits appropriate events
The error handling and event emission are comprehensive.
113-203: Clean implementation for ERC20-originated tokens.The function properly handles the conversion by:
- Transferring ERC20 tokens to the EVM module
- Minting equivalent bank coins
- Sending the minted coins to the recipient
The use of
balIncreasefrom the Transfer call ensures accurate coin minting.
205-303: Comprehensive WNIBI conversion with proper validation.The function correctly handles WNIBI-specific conversion including:
- Validation that the contract exists (non-zero code size)
- Balance verification before and after withdrawal
- Proper wei to micronibi conversion
x/evm/evmmodule/genesis_test.go (1)
142-147: Should the sender’s ERC20 balance reflect the EVM→bank conversion?If ConvertEvmToCoin burns/transfers from Sender’s EVM balance, expected should subtract amountToSendC as well. The current assertion ignores it.
If the burn/transfer is intended, update:
-sumAB := new(big.Int).Add(new(big.Int).Set(amountToSendA), amountToSendB) +sumAB := new(big.Int).Add(new(big.Int).Set(amountToSendA), amountToSendB) +sumABC := new(big.Int).Add(sumAB, amountToSendC) -expected := new(big.Int).Sub(new(big.Int).Set(totalSupply), sumAB) +expected := new(big.Int).Sub(new(big.Int).Set(totalSupply), sumABC)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/upgrades/v2_7_0/v2_7_0.go (1)
50-58: Make CanonicalWnibi update idempotent and mainnet-onlyAvoid overwriting non-mainnet/test settings and skip no-op writes. Add logging.
func AddWnibiToNibiruEvm( keepers *keepers.PublicKeepers, ctx sdk.Context, ) error { - wnibiAddrMainnet := appconst.MAINNET_WNIBI_ADDR - evmParams := keepers.EvmKeeper.GetParams(ctx) - evmParams.CanonicalWnibi = eth.EIP55Addr{ - Address: wnibiAddrMainnet, - } - - err := keepers.EvmKeeper.SetParams(ctx, evmParams) - return err + if ctx.ChainID() != "nibiru-1" { + ctx.Logger().Info("Skipping CanonicalWnibi update: not mainnet", "chain_id", ctx.ChainID()) + return nil + } + target := eth.EIP55Addr{Address: appconst.MAINNET_WNIBI_ADDR} + evmParams := keepers.EvmKeeper.GetParams(ctx) + if evmParams.CanonicalWnibi.Equal(target) { + return nil + } + // If already set to a different non-zero value, log and skip to be safe. + if !evmParams.CanonicalWnibi.Address.IsZero() && !evmParams.CanonicalWnibi.Equal(target) { + ctx.Logger().Info("CanonicalWnibi already set to non-target; skipping", + "existing", evmParams.CanonicalWnibi.Hex(), "target", target.Hex()) + return nil + } + evmParams.CanonicalWnibi = target + if err := keepers.EvmKeeper.SetParams(ctx, evmParams); err != nil { + return err + } + ctx.Logger().Info("CanonicalWnibi parameter set", "address", target.Hex()) + return nil }x/evm/evmmodule/genesis_test.go (1)
128-133: Recreate EVM after reinitializing modules to avoid stale stateAfter deps is replaced and genesis reinitialized, evmObj still points to the old state/config.
evmmodule.InitGenesis(deps.Ctx, deps.EvmKeeper, deps.App.AccountKeeper, *evmGenesisState) + // Recreate EVM bound to the new deps/context + evmObj, _ = deps.NewEVM()
🧹 Nitpick comments (13)
x/evm/keeper/keeper_test.go (2)
20-23: Consider separate Test functions per suite for grepability and -run filtering.Running two suites from a single
TestAllworks, but defining oneTestXxxper suite improves dev ergonomics (go test -run TestSuiteFunTokenetc.) and isolates reporting.-func TestAll(t *testing.T) { - suite.Run(t, new(Suite)) - suite.Run(t, new(SuiteFunToken)) -} +func TestSuite(t *testing.T) { + suite.Run(t, new(Suite)) +} + +func TestSuiteFunToken(t *testing.T) { + suite.Run(t, new(SuiteFunToken)) +}
161-169: Harden TestGetHashFn: avoid uint64 underflow and strengthen assertions.Casting to
uint64before subtracting can underflow when height is 0. Also, assert the header hash is non-zero to make the positive case stronger.func (s *Suite) TestGetHashFn() { deps := evmtest.NewTestDeps() fn := deps.EvmKeeper.GetHashFn(deps.Ctx) s.Equal(gethcommon.Hash{}, fn(math.MaxInt64+1)) - s.Equal(gethcommon.BytesToHash(deps.Ctx.HeaderHash()), fn(uint64(deps.Ctx.BlockHeight()))) - s.Equal(gethcommon.Hash{}, fn(uint64(deps.Ctx.BlockHeight())+1)) - s.Equal(gethcommon.Hash{}, fn(uint64(deps.Ctx.BlockHeight())-1)) + h := deps.Ctx.BlockHeight() + cur := gethcommon.BytesToHash(deps.Ctx.HeaderHash()) + s.NotEqual(gethcommon.Hash{}, cur) // ensure header hash is non-zero + s.Equal(cur, fn(uint64(h))) + s.Equal(gethcommon.Hash{}, fn(uint64(h)+1)) + if h > 0 { + s.Equal(gethcommon.Hash{}, fn(uint64(h-1))) + } else { + s.Equal(gethcommon.Hash{}, fn(0)) + } }x/evm/evmtest/genesis_inject.go (4)
27-31: Prefer app codec over stdlib for genesis decodingUse the app’s codec to avoid drift with custom (de)serializers and future tag changes.
- var genAcc evm.GenesisAccount - err := json.Unmarshal([]byte(WNIBI_GENESIS_ACC_STRING), &genAcc) + encCfg := app.MakeEncodingConfig() + var genAcc evm.GenesisAccount + err := encCfg.Codec.UnmarshalJSON([]byte(WNIBI_GENESIS_ACC_STRING), &genAcc)
47-55: Validate bech32 matches MAINNET_WNIBI_ADDR (or derive only from source of truth)You hardcode the EthAccount JSON address string elsewhere and also derive it here from MAINNET_WNIBI_ADDR. Keep a single source to prevent drift; optionally assert they match at init.
genAcc := eth.EthAccount{ BaseAccount: &auth.BaseAccount{ - Address: eth.EthAddrToNibiruAddr(appconst.MAINNET_WNIBI_ADDR).String(), + Address: eth.EthAddrToNibiruAddr(appconst.MAINNET_WNIBI_ADDR).String(), PubKey: nil, AccountNumber: 13, Sequence: 0, }, CodeHash: "0xffb88e0eb48147949565e65de3ec8a54b746214da7b9dd5b9a8a3ae7df46193b", } + // Optional sanity check if you decide to keep the JSON constant below: + // if !strings.Contains(WNIBI_GENESIS_AUTH_ACC_STRING, genAcc.BaseAccount.Address) { panic("WNIBI bech32 mismatch") }
56-61: Remove commented-out codeDead/commented parsing paths add noise and can get stale.
- // err := encCfg.Codec.Unmarshal([]byte(WNIBI_GENESIS_AUTH_ACC_STRING), &genAcc) - // // err := json.Unmarshal([]byte(WNIBI_GENESIS_AUTH_ACC_STRING), &genAcc) - // if err != nil { - // panic(fmt.Errorf("failed to unpack WNIBI_GENESIS_AUTH_ACC_STRING as eth.EthAccount: %w", err)) - // }
65-76: Drop unused WNIBI_GENESIS_AUTH_ACC_STRING or use it consistentlyThis constant isn’t consumed and risks diverging from MAINNET_WNIBI_ADDR. Either delete it or make it the single source of truth.
-const WNIBI_GENESIS_AUTH_ACC_STRING = ` -{ - "@type": "/eth.types.v1.EthAccount", - "base_account": { - "address": "nibi1pjk0v60cg347e2pxjyarc6uk4n2tq25hhymgg9", - "pub_key": null, - "account_number": "13", - "sequence": "0" - }, - "code_hash": "0xffb88e0eb48147949565e65de3ec8a54b746214da7b9dd5b9a8a3ae7df46193b" -} -` +// (Optional) keep no duplicate JSON sources for the same account to avoid drift.x/common/testutil/genesis/genesis.go (5)
28-37: Guard against nil VotingPeriod pointerUse an explicit duration value; don’t assume pointer is non-nil.
- *govGenState.Params.VotingPeriod = 20 * time.Second + d := 20 * time.Second + govGenState.Params.VotingPeriod = &d
46-57: Avoid duplicate EVM genesis accounts (idempotency)If tests call this more than once, duplicates break genesis. Check before append.
- evmGenState.Accounts = append(evmGenState.Accounts, evmtest.WNIBI_GENESIS_EVM_ACC) + exists := false + for _, acc := range evmGenState.Accounts { + if strings.EqualFold(acc.Address, evmtest.WNIBI_GENESIS_EVM_ACC.Address) { + exists = true + break + } + } + if !exists { + evmGenState.Accounts = append(evmGenState.Accounts, evmtest.WNIBI_GENESIS_EVM_ACC) + }Note: add
stringsto imports.
73-77: Check PackAccounts error and avoid append if packing failsDon’t ignore the error even if it “can’t happen”.
- protobufAnyAccs, _ := auth.PackAccounts(auth.GenesisAccounts{ - &wnibiAcc, - }) + protobufAnyAccs, err := auth.PackAccounts(auth.GenesisAccounts{&wnibiAcc}) + if err != nil { + panic(fmt.Errorf("failed to pack WNIBI EthAccount: %w", err)) + }
77-84: Avoid duplicate Auth accounts (same address)Check existing accounts before appending WNIBI to keep genesis idempotent.
- authGenState.Accounts = append(authGenState.Accounts, wnibiAccAsProtobufAny) + // naive check by bech32 string match to avoid unpacking all Any's in tests + addr := wnibiAcc.BaseAccount.Address + already := false + for _, anyAcc := range authGenState.Accounts { + if strings.Contains(string(anyAcc.Value), addr) { + already = true + break + } + } + if !already { + authGenState.Accounts = append(authGenState.Accounts, wnibiAccAsProtobufAny) + }Note: add
stringsto imports.
20-23: Doc comment nitMinor: collapse multi-line sentence into one line per Go doc convention.
-// NewTestGenesisState returns [app.GenesisState] using the default genesis as input. -// The blockchain genesis state is represented as a map from module identifier -// strings to raw json messages. +// NewTestGenesisState returns a default app.GenesisState (map[module]json.RawMessage) with test-friendly tweaks.x/evm/keeper/msg_convert_evm_to_coin.go (1)
48-64: Minor: deduplicate core.Message scaffolding
These three blocks share identical zero-gas scaffolding. Extract a small helper to build messages to reduce duplication and drift.Also applies to: 134-150, 235-251
x/evm/evmmodule/genesis_test.go (1)
101-101: Fix direction in commentThis test invokes ConvertEvmToCoin (EVM → bank), not bank → EVM.
- // Send fungible token coins from bank to evm + // Convert ERC20 tokens to bank coins (EVM → bank)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/upgrades/v2_7_0/v2_7_0.go(1 hunks)x/common/testutil/genesis/genesis.go(1 hunks)x/evm/evmmodule/genesis_test.go(5 hunks)x/evm/evmtest/genesis_inject.go(1 hunks)x/evm/keeper/keeper_test.go(2 hunks)x/evm/keeper/msg_convert_evm_to_coin.go(1 hunks)x/evm/keeper/msg_server.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/keeper/msg_server.go
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-01-17T17:32:27.341Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns only `*vm.EVM` without an error value.
Applied to files:
x/evm/evmmodule/genesis_test.go
📚 Learning: 2025-01-17T17:32:27.341Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns `(*vm.EVM, *statedb.StateDB)` without an error value. The second return value is often discarded when only the EVM object is needed.
Applied to files:
x/evm/evmmodule/genesis_test.go
📚 Learning: 2025-01-17T17:32:33.412Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns only an EVM object and does not return an error. The method is used in test files to create a new EVM instance for testing purposes.
Applied to files:
x/evm/evmmodule/genesis_test.go
📚 Learning: 2025-01-17T17:32:33.412Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns both an EVM object and a StateDB object with the signature `(*vm.EVM, *statedb.StateDB)`. The method is used in test files to create new EVM and StateDB instances for testing purposes.
Applied to files:
x/evm/evmmodule/genesis_test.go
📚 Learning: 2024-12-06T16:09:01.767Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2119
File: x/evm/keeper/bank_extension_test.go:161-165
Timestamp: 2024-12-06T16:09:01.767Z
Learning: In Go test assertions within `x/evm/keeper/bank_extension_test.go`, when comparing `uint64` values using `s.Equalf`, prefer formatting the values as decimal strings with `fmt.Sprintf("%d", value)` to avoid them being printed in hexadecimal.
Applied to files:
x/evm/evmmodule/genesis_test.go
📚 Learning: 2024-10-31T18:28:26.466Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2095
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-10-31T18:28:26.466Z
Learning: When implementing the `bankkeeper.Keeper` interface in `x/evm/keeper/bank_extension.go`, methods must use non-pointer receivers to satisfy the interface requirements.
Applied to files:
x/evm/keeper/msg_convert_evm_to_coin.go
🧬 Code graph analysis (6)
x/common/testutil/genesis/genesis.go (6)
app/genesis.go (1)
GenesisState(74-74)app/app.go (1)
ModuleBasics(122-161)x/evm/const.go (1)
ModuleName(41-41)x/common/denoms/denoms.go (1)
NIBI(23-23)x/common/testutil/testapp/testapp.go (1)
SetDefaultSudoGenesis(79-91)x/evm/evmtest/genesis_inject.go (2)
WNIBI_GENESIS_EVM_ACC(18-18)WNIBI_GENESIS_AUTH_ACC(20-20)
x/evm/keeper/keeper_test.go (1)
x/evm/evmtest/test_deps.go (1)
NewTestDeps(36-47)
app/upgrades/v2_7_0/v2_7_0.go (4)
app/upgrades/v2_5_0/v2_5_0.go (2)
Upgrade(31-53)UpgradeName(29-29)app/keepers/all_keepers.go (1)
PublicKeepers(35-75)app/appconst/appconst.go (1)
MAINNET_WNIBI_ADDR(34-34)eth/eip55.go (1)
EIP55Addr(17-19)
x/evm/evmmodule/genesis_test.go (6)
x/evm/evmtest/test_deps.go (1)
NewTestDeps(36-47)x/evm/evm.pb.go (3)
FunToken(32-41)FunToken(45-45)FunToken(46-48)x/evm/tx.pb.go (6)
MsgCreateFunToken(480-487)MsgCreateFunToken(491-491)MsgCreateFunToken(492-494)MsgConvertEvmToCoin(675-693)MsgConvertEvmToCoin(697-697)MsgConvertEvmToCoin(698-700)eth/eip55.go (2)
EIP55Addr(17-19)NewEIP55AddrFromStr(22-35)x/evm/evmtest/erc20.go (1)
FunTokenBalanceAssert(153-159)x/evm/evmmodule/genesis.go (2)
ExportGenesis(86-129)InitGenesis(20-83)
x/evm/evmtest/genesis_inject.go (2)
eth/eth_account.go (1)
EthAddrToNibiruAddr(15-17)app/appconst/appconst.go (1)
MAINNET_WNIBI_ADDR(34-34)
x/evm/keeper/msg_convert_evm_to_coin.go (8)
x/evm/statedb/statedb.go (1)
StateDB(45-85)x/evm/embeds/embeds.go (2)
SmartContract_ERC20MinterWithMetadataUpdates(71-74)SmartContract_WNIBI(96-99)x/evm/const.go (2)
EVM_MODULE_ADDRESS(106-106)WeiToNative(136-139)x/evm/keeper/erc20.go (1)
Erc20GasLimitExecute(30-30)eth/eip55.go (1)
EIP55Addr(17-19)x/evm/keeper/msg_server.go (1)
ParseWeiAsMultipleOfMicronibi(436-464)x/evm/errors.go (1)
ErrCanonicalWnibi(54-54)app/appconst/appconst.go (1)
BondDenom(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: proto-gen-rs
- GitHub Check: build
- GitHub Check: unit-tests
- GitHub Check: integration-tests
- GitHub Check: e2e-evm
- GitHub Check: lint
🔇 Additional comments (7)
x/evm/keeper/keeper_test.go (4)
4-4: Imports look correct for new assertions.
mathis used for the out-of-range sentinel andgethcommonfor hash conversions. No concerns.Also applies to: 8-8
16-18: Section dividers are style-only.No action needed; just noting the added comment separators.
Also applies to: 33-34
161-169: Confirm intended semantics vs EVM BLOCKHASH.The test asserts zero for the previous height. If
GetHashFnis meant to emulate the EVMBLOCKHASHopcode, prior blocks within the 256-block window typically return a non-zero hash (excluding the current block). If the zero-for-previous behavior is intentional for Nibiru, consider a brief comment in the test to document the divergence; otherwise, add coverage for the 256-block window.
29-31: No-op suite warning resolved SuiteFunToken has multiple Test* methods in other_test.gofiles (e.g. funtoken_from_erc20_test.go, msg_convert_coin_to_evm_test.go); no further action needed.x/common/testutil/genesis/genesis.go (1)
63-84: Remove EthAccount shape verification EthAccount embedsBaseAccount, soAccountNumberis promoted and accessible—wnibiAcc.AccountNumber = …is valid.Likely an incorrect or invalid review comment.
x/evm/keeper/msg_convert_evm_to_coin.go (1)
132-152: Confirm msg.sender for ERC20 transfer path (sender vs module addr)evmObj is built with From: EVM_MODULE_ADDRESS and SkipFromEOACheck: true, but the subsequent ERC20().Transfer is semantically initiated by senderEthAddr. If Transfer relies on msg.sender, this can misattribute the call.
Consider aligning with the WNIBI path and constructing evmObj from senderEthAddr:
- evmMsg := core.Message{ - To: &erc20Addr, - From: evm.EVM_MODULE_ADDRESS, + evmMsg := core.Message{ + To: &erc20Addr, + From: senderEthAddr, Nonce: k.GetAccNonce(ctx, evm.EVM_MODULE_ADDRESS), Value: unusedBigInt, GasLimit: Erc20GasLimitExecute, GasPrice: unusedBigInt, GasFeeCap: unusedBigInt, GasTipCap: unusedBigInt, Data: contractInput, AccessList: gethcore.AccessList{}, BlobGasFeeCap: &big.Int{}, BlobHashes: []gethcommon.Hash{}, - SkipNonceChecks: true, - SkipFromEOACheck: true, + SkipNonceChecks: false, + SkipFromEOACheck: false, }If ERC20().Transfer internally sets the from-account irrespective of evmObj, please confirm and ignore this change. Otherwise, this prevents accidental transfers from the module’s balance.
x/evm/evmmodule/genesis_test.go (1)
104-110: Recipient format: verify ToAddr form used in testsProto allows hex or bech32 for ToAddr. You’re passing EIP-55 hex. If downstream assertions expect bech32 events/balances, consider using the bech32 string of the bank recipient to match keeper expectations and event payloads.
Optionally:
- ToAddr: eip55Addr.Hex(), + ToAddr: deps.Sender.NibiruAddr.String(), // or the intended bech32 recipient
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
x/evm/keeper/msg_convert_coin_to_evm.go (6)
52-58: StateDB lifecycle bug: clobbers existing pointer and never restores it.You create a StateDB when nil, but you never set k.Bank.StateDB to it, and you always defer setting k.Bank.StateDB = nil, which can erase a previously-set StateDB from upstream callers. Restore previous pointer and only commit when you created the StateDB. (Same issue recurs below.)
- stateDB := k.Bank.StateDB - if stateDB == nil { - stateDB = k.NewStateDB(ctx, k.TxConfig(ctx, gethcommon.Hash{})) - } - defer func() { - k.Bank.StateDB = nil - }() + prevStateDB := k.Bank.StateDB + stateDB := prevStateDB + createdState := false + if stateDB == nil { + stateDB = k.NewStateDB(ctx, k.TxConfig(ctx, gethcommon.Hash{})) + k.Bank.StateDB = stateDB + createdState = true + } + defer func() { + // Restore whatever was there before. + k.Bank.StateDB = prevStateDB + }()
160-164: Commit only if this function created the StateDB.Avoid double-commits and committing someone else’s StateDB.
- if err := stateDB.Commit(); err != nil { - return nil, fmt.Errorf("failed to commit stateDB: %w", err) - } + if createdState { + if err := stateDB.Commit(); err != nil { + return nil, fmt.Errorf("failed to commit stateDB: %w", err) + } + }
216-223: Repeat: StateDB lifecycle (born-coin path).Same pointer clobber/restore issue as above. Apply the same pattern.
- stateDB := k.Bank.StateDB - if stateDB == nil { - stateDB = k.NewStateDB(ctx, txConfig) - } - defer func() { - k.Bank.StateDB = nil - }() + prevStateDB := k.Bank.StateDB + stateDB := prevStateDB + createdState := false + if stateDB == nil { + stateDB = k.NewStateDB(ctx, txConfig) + k.Bank.StateDB = stateDB + createdState = true + } + defer func() { k.Bank.StateDB = prevStateDB }()
244-246: Commit only if created (born-coin path).- if err = stateDB.Commit(); err != nil { + if createdState { + if err = stateDB.Commit(); err != nil { return nil, sdkioerrors.Wrap(err, "failed to commit stateDB") - } + } + }
276-283: Repeat: StateDB lifecycle (born-ERC20 path).Mirror the same fix here.
- stateDB := k.Bank.StateDB - if stateDB == nil { - stateDB = k.NewStateDB(ctx, k.TxConfig(ctx, gethcommon.Hash{})) - } - defer func() { - k.Bank.StateDB = nil - }() + prevStateDB := k.Bank.StateDB + stateDB := prevStateDB + createdState := false + if stateDB == nil { + stateDB = k.NewStateDB(ctx, k.TxConfig(ctx, gethcommon.Hash{})) + k.Bank.StateDB = stateDB + createdState = true + } + defer func() { k.Bank.StateDB = prevStateDB }()
348-350: Commit only if created (born-ERC20 path).- if err := stateDB.Commit(); err != nil { - return nil, sdkioerrors.Wrap(err, "failed to commit stateDB") - } + if createdState { + if err := stateDB.Commit(); err != nil { + return nil, sdkioerrors.Wrap(err, "failed to commit stateDB") + } + }x/evm/keeper/msg_convert_evm_to_coin.go (1)
36-37: Fix 18→6 decimal conversion: over-crediting bank coins by 1e12For coin-originated tokens,
amountis in wei (18dp). Bank coins use micro (6dp). Convert withevm.WeiToNativebefore constructing the sdk.Coin.- bankCoins := sdk.NewCoins(sdk.NewCoin(bankDenom, sdkmath.NewIntFromBigInt(amount))) + // Convert 18-decimal ERC20 amount -> 6-decimal bank coin amount + bankAmt := sdkmath.NewIntFromBigInt(evm.WeiToNative(amount)) + bankCoins := sdk.NewCoins(sdk.NewCoin(bankDenom, bankAmt))
🧹 Nitpick comments (12)
CHANGELOG.md (1)
50-54: Clarify wording and wrap continuation lines for consistency
- Prefer “Cosmos SDK transaction (non-EVM)” over “non-Ethereum transaction” for precision.
- Indent wrapped lines by two spaces to match multi-line bullets elsewhere in this file.
- - [#2345](https://github.com/NibiruChain/nibiru/pull/2345) - feat(evm): add -"eth.evm.v1.MsgConvertEvmToCoin" tx for ERC20 to bank coin conversions with a -non-Ethereum transaction. This change introduces new message types, CLI commands, -and keeper logic to cover both directions of conversion, including special-case -handling for NIBI via WNIBI. + - [#2345](https://github.com/NibiruChain/nibiru/pull/2345) - feat(evm): add + "eth.evm.v1.MsgConvertEvmToCoin" tx for ERC20 to bank coin conversions with a + Cosmos SDK transaction (non-EVM). This change introduces new message types, CLI commands, + and keeper logic to cover both directions of conversion, including special-case + handling for NIBI via WNIBI.x/evm/msg.go (2)
409-419: Fix wrong value in error message (diagnostics).On non-Ethereum messages, the error prints the sdk.Tx pointer type instead of the offending msg type.
- return nil, fmt.Errorf("invalid tx type: %T", tx) + return nil, fmt.Errorf("invalid msg type: %T", msg)
542-584: Tighten Erc20 emptiness check and improve ToAddr error context.
- Use the same emptiness idiom as other EIP55 fields for consistency.
- Include the bad input in the ToAddr error for easier debugging.
- if (m.Erc20Addr.Address == gethcommon.Address{}) { - err = fmt.Errorf("empty erc20_addr") - return - } + if m.Erc20Addr.String() == "" || m.Erc20Addr.Size() == 0 { + err = fmt.Errorf("empty erc20_addr") + return + } @@ - toAddr.Bech32, err = sdk.AccAddressFromBech32(m.ToAddr) + toAddr.Bech32, err = sdk.AccAddressFromBech32(m.ToAddr) if err != nil { - err = fmt.Errorf("invalid bech32 or hex address: %w", err) + err = fmt.Errorf("invalid bech32 or hex address %q: %w", m.ToAddr, err) return }x/evm/keeper/msg_convert_coin_to_evm.go (4)
116-121: Error message direction is inverted.This path performs NIBI -> WNIBI deposit, but the message says “convert WNIBI to NIBI”.
- return resp, fmt.Errorf("failed to convert WNIBI to NIBI: %w", err) + return resp, fmt.Errorf("failed to deposit NIBI into WNIBI: %w", err) @@ - err = fmt.Errorf("failed to convert WNIBI to NIBI: VmError: %s", evmResp.VmError) + err = fmt.Errorf("failed to deposit NIBI into WNIBI: VmError: %s", evmResp.VmError)
134-136: Typos in comment.“tranfers” -> “transfers”.
- // STEP 2: Sender tranfers WNIBI to intended recipient + // STEP 2: Sender transfers WNIBI to intended recipient
145-157: Error wording + typo (“receipient”).This step is an ERC20 transfer of WNIBI. Adjust wording; fix spelling.
- return resp, fmt.Errorf("failed to convert WNIBI to NIBI: %w", err) + return resp, fmt.Errorf("failed to transfer WNIBI to recipient: %w", err) @@ - err = fmt.Errorf("failed to convert WNIBI to NIBI: VmError: %s", evmResp.VmError) + err = fmt.Errorf("failed to transfer WNIBI to recipient: VmError: %s", evmResp.VmError) @@ - "ConvertCoinToEvm: transfer of WNIBI succeeded but did not deliver the correct number of tokens: transfer amount %s, balance increase %s, senderHex %s, receipient %s", + "ConvertCoinToEvm: WNIBI transfer succeeded but delivered incorrect amount: expected %s, got %s, sender %s, recipient %s",
136-144: Optional: emit tx logs for parity and indexing.In WNIBI branch, you don’t emit EventTxLog nor update the bloom, unlike the other branches. Consider emitting logs to keep indexing behavior consistent.
evmResp, err := k.CallContractWithInput( @@ if err != nil { ... } + // Emit tx logs and update bloom like other paths + _ = ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{Logs: evmResp.Logs}) + k.updateBlockBloom(ctx, evmResp, uint64(k.EvmState.BlockTxIndex.GetOr(ctx, 0)))Also applies to: 256-260
x/evm/keeper/msg_convert_evm_to_coin.go (5)
318-324: Emit EventTxLog and update bloom for WNIBI path (parity with other paths)Add tx-log emission and bloom update so indexers/observers see the WNIBI withdraw logs consistently.
_ = ctx.EventManager().EmitTypedEvent(&evm.EventConvertEvmToCoin{ Sender: sender.Bech32.String(), Erc20ContractAddress: erc20.Hex(), ToAddress: toAddrBech32.String(), BankCoin: withdrawnMicronibi, SenderEthAddr: sender.Eth.Hex(), }) + // Emit tx logs and update bloom (match other conversion paths) + if evmResp != nil { + if err := ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{Logs: evmResp.Logs}); err == nil { + k.updateBlockBloom(ctx, evmResp, uint64(k.EvmState.BlockTxIndex.GetOr(ctx, 0))) + } + }
95-103: Normalize contract address formatting in events (.Hex vs .String)Use
.Hex()consistently across paths to avoid mixed casing/formatting.- Erc20ContractAddress: erc20Addr.String(), + Erc20ContractAddress: erc20Addr.Hex(),- Erc20ContractAddress: erc20Addr.String(), + Erc20ContractAddress: erc20Addr.Hex(),Also applies to: 188-194
104-108: Decouple bloom update from event emission errorBloom should update regardless of EventTxLog emission outcome.
- err = ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{Logs: evmResp.Logs}) - if err == nil { - k.updateBlockBloom(ctx, evmResp, uint64(k.EvmState.BlockTxIndex.GetOr(ctx, 0))) - } + _ = ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{Logs: evmResp.Logs}) + k.updateBlockBloom(ctx, evmResp, uint64(k.EvmState.BlockTxIndex.GetOr(ctx, 0)))Also applies to: 196-200
23-27: Tighten Go doc comment styleStart with the function name and remove blank lines for proper GoDoc rendering.
-// convertEvmToCoinForCoinOriginated is part of the -// -// "eth.evm.v1.MsgConvertEvmToCoin" tx. This function handles conversion of ERC20 -// -// tokens that were originally bank coins back into coin form. The EVM module -// -// owns the ERC20 contract and will burn the tokens +// convertEvmToCoinForCoinOriginated handles "eth.evm.v1.MsgConvertEvmToCoin" +// for tokens that were originally bank coins. The EVM module owns the ERC20 +// contract and burns the tokens before releasing bank coins.
47-63: Reduce duplication: factor EVM message builderThree near-identical
core.Messageconstructions. Consider a small helper to set zero-valued fee fields and common flags.Also applies to: 134-150, 241-257
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/evm/events.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
CHANGELOG.md(2 hunks)api/eth/evm/v1/events.pulsar.go(8 hunks)proto/eth/evm/v1/events.proto(4 hunks)x/evm/keeper/msg_convert_coin_to_evm.go(1 hunks)x/evm/keeper/msg_convert_coin_to_evm_test.go(1 hunks)x/evm/keeper/msg_convert_evm_to_coin.go(1 hunks)x/evm/keeper/msg_convert_evm_to_coin_test.go(1 hunks)x/evm/keeper/msg_server.go(7 hunks)x/evm/msg.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- x/evm/keeper/msg_convert_evm_to_coin_test.go
- x/evm/keeper/msg_convert_coin_to_evm_test.go
- proto/eth/evm/v1/events.proto
- x/evm/keeper/msg_server.go
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-10-08T15:45:59.488Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#1680
File: x/perp/v2/keeper/msg_server.go:210-210
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The user indicated that error handling for `sdk.AccAddressFromBech32(msg.Sender)` is covered by prior `ValidateBasic` checks in the code flow.
Applied to files:
x/evm/msg.go
📚 Learning: 2024-10-08T15:45:59.488Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#1680
File: x/perp/v2/keeper/msg_server.go:198-198
Timestamp: 2024-10-08T15:45:59.488Z
Learning: The user Unique-Divine has indicated that the error check for `sdk.AccAddressFromBech32(msg.Sender)` is not necessary in the context of their code because it is covered by the `ValidateBasic` method.
Applied to files:
x/evm/msg.go
📚 Learning: 2024-12-06T16:09:01.767Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2119
File: x/evm/keeper/bank_extension_test.go:161-165
Timestamp: 2024-12-06T16:09:01.767Z
Learning: In Go test assertions within `x/evm/keeper/bank_extension_test.go`, when comparing `uint64` values using `s.Equalf`, prefer formatting the values as decimal strings with `fmt.Sprintf("%d", value)` to avoid them being printed in hexadecimal.
Applied to files:
x/evm/keeper/msg_convert_evm_to_coin.go
📚 Learning: 2024-12-06T11:54:38.275Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
Applied to files:
x/evm/keeper/msg_convert_evm_to_coin.go
📚 Learning: 2024-10-31T18:28:26.466Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2095
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-10-31T18:28:26.466Z
Learning: When implementing the `bankkeeper.Keeper` interface in `x/evm/keeper/bank_extension.go`, methods must use non-pointer receivers to satisfy the interface requirements.
Applied to files:
x/evm/keeper/msg_convert_evm_to_coin.go
🧬 Code graph analysis (4)
api/eth/evm/v1/events.pulsar.go (1)
x/evm/events.pb.go (3)
EventConvertEvmToCoin(528-534)EventConvertEvmToCoin(538-538)EventConvertEvmToCoin(539-541)
x/evm/keeper/msg_convert_coin_to_evm.go (9)
x/evm/tx.pb.go (6)
MsgConvertCoinToEvm(582-589)MsgConvertCoinToEvm(593-593)MsgConvertCoinToEvm(594-596)MsgConvertCoinToEvmResponse(638-639)MsgConvertCoinToEvmResponse(643-643)MsgConvertCoinToEvmResponse(644-646)eth/eth_account.go (1)
NibiruAddrToEthAddr(19-21)x/evm/statedb/statedb.go (1)
StateDB(45-85)x/evm/statedb/config.go (1)
TxConfig(15-20)x/evm/errors.go (1)
ErrCanonicalWnibi(54-54)x/evm/const.go (2)
NativeToWei(123-126)EVM_MODULE_ADDRESS(106-106)x/evm/embeds/embeds.go (2)
SmartContract_WNIBI(96-99)SmartContract_ERC20MinterWithMetadataUpdates(71-74)x/evm/keeper/erc20.go (1)
Erc20GasLimitExecute(30-30)x/evm/events.pb.go (6)
EventConvertCoinToEvm(291-296)EventConvertCoinToEvm(300-300)EventConvertCoinToEvm(301-303)EventTxLog(128-131)EventTxLog(135-135)EventTxLog(136-138)
x/evm/msg.go (4)
x/evm/tx.pb.go (3)
MsgConvertEvmToCoin(675-693)MsgConvertEvmToCoin(697-697)MsgConvertEvmToCoin(698-700)eth/eip55.go (2)
EIP55Addr(17-19)NewEIP55AddrFromStr(22-35)eth/eth_account.go (2)
NibiruAddrToEthAddr(19-21)EthAddrToNibiruAddr(15-17)x/evm/codec.go (1)
AminoCdc(18-18)
x/evm/keeper/msg_convert_evm_to_coin.go (8)
x/evm/msg.go (1)
Addrs(537-540)x/evm/statedb/statedb.go (1)
StateDB(45-85)x/evm/embeds/embeds.go (2)
SmartContract_ERC20MinterWithMetadataUpdates(71-74)SmartContract_WNIBI(96-99)x/evm/const.go (2)
EVM_MODULE_ADDRESS(106-106)WeiToNative(136-139)x/evm/keeper/erc20.go (1)
Erc20GasLimitExecute(30-30)x/evm/keeper/msg_server.go (1)
ParseWeiAsMultipleOfMicronibi(435-463)x/evm/errors.go (1)
ErrCanonicalWnibi(54-54)app/appconst/appconst.go (1)
BondDenom(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build
- GitHub Check: unit-tests
- GitHub Check: e2e-evm
- GitHub Check: integration-tests
- GitHub Check: lint
🔇 Additional comments (10)
CHANGELOG.md (5)
42-42: LGTM: spacing after the Unreleased headerKeeps sections readable and matches existing style.
43-48: LGTM: CI note formattingComment is clear and properly closed. No action needed.
65-65: LGTM: corrected package link and descriptionReads well and links look correct.
68-68: LGTM: PR link fixThe PR reference now points to the correct URL.
77-77: LGTM: blank line before DependenciesConsistent with nearby sections; improves readability.
x/evm/msg.go (4)
300-301: LGTM: safer From-address extractionUsing gethcommon.HexToAddress(msg.From).Bytes() is consistent with upstream geth utils and ValidateBasic already guards invalid From.
321-331: LGTM: return type now gethcommon.AddressChanging GetSender to return gethcommon.Address aligns better with geth APIs and avoids extra conversions.
510-522: LGTM: clearer validation for MsgConvertCoinToEvmImproved error messages and explicit empty checks read well.
586-595: LGTM: ValidateBasic delegates to ValidateKeeps basic checks self-contained and stateless.
api/eth/evm/v1/events.pulsar.go (1)
4480-5940: LGTM: protobuf surface for EventConvertEvmToCoin added correctlyNew message, reflection scaffolding, descriptors, and dependency indices look consistent with the proto definition. No manual edits recommended in generated code.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
x/evm/keeper/msg_convert_evm_to_coin.go (3)
36-37: Fix 18→6 decimal conversion for coin-originated tokens (over-credit by 1e12).Convert ERC20 wei to micro-denom before constructing bank coins.
- bankCoins := sdk.NewCoins(sdk.NewCoin(bankDenom, sdkmath.NewIntFromBigInt(amount))) + bankAmt := sdkmath.NewIntFromBigInt(evm.WeiToNative(amount)) + bankCoins := sdk.NewCoins(sdk.NewCoin(bankDenom, bankAmt))
168-174: Fix 18→6 decimal conversion when minting for ERC20-originated tokens.Mint/send micro-denom, not wei.
- bankCoin := sdk.NewCoin(bankDenom, sdkmath.NewIntFromBigInt(balIncrease)) + bankAmt := sdkmath.NewIntFromBigInt(evm.WeiToNative(balIncrease)) + bankCoin := sdk.NewCoin(bankDenom, bankAmt)
317-325: Emit EventTxLog and update bloom for WNIBI path (parity with other paths).Improves observability and indexer parity.
_ = ctx.EventManager().EmitTypedEvent(&evm.EventConvertEvmToCoin{ Sender: sender.Bech32.String(), Erc20ContractAddress: erc20.Hex(), ToAddress: toAddrBech32.String(), BankCoin: withdrawnMicronibi, SenderEthAddr: sender.Eth.Hex(), }) + // Emit tx logs and update bloom (mirrors other conversion paths) + if err := ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{Logs: evmResp.Logs}); err == nil { + k.updateBlockBloom(ctx, evmResp, uint64(k.EvmState.BlockTxIndex.GetOr(ctx, 0))) + }
🧹 Nitpick comments (1)
x/evm/keeper/msg_convert_evm_to_coin.go (1)
95-102: Unify contract address formatting in events.Use Hex() consistently across paths.
- Erc20ContractAddress: erc20Addr.String(), + Erc20ContractAddress: erc20Addr.Hex(),- Erc20ContractAddress: erc20Addr.String(), + Erc20ContractAddress: erc20Addr.Hex(),Also applies to: 187-194
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(2 hunks)x/evm/keeper/msg_convert_evm_to_coin.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-12-06T16:09:01.767Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2119
File: x/evm/keeper/bank_extension_test.go:161-165
Timestamp: 2024-12-06T16:09:01.767Z
Learning: In Go test assertions within `x/evm/keeper/bank_extension_test.go`, when comparing `uint64` values using `s.Equalf`, prefer formatting the values as decimal strings with `fmt.Sprintf("%d", value)` to avoid them being printed in hexadecimal.
Applied to files:
x/evm/keeper/msg_convert_evm_to_coin.go
📚 Learning: 2024-12-06T11:54:38.275Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
Applied to files:
x/evm/keeper/msg_convert_evm_to_coin.go
📚 Learning: 2024-10-31T18:28:26.466Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2095
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-10-31T18:28:26.466Z
Learning: When implementing the `bankkeeper.Keeper` interface in `x/evm/keeper/bank_extension.go`, methods must use non-pointer receivers to satisfy the interface requirements.
Applied to files:
x/evm/keeper/msg_convert_evm_to_coin.go
🧬 Code graph analysis (1)
x/evm/keeper/msg_convert_evm_to_coin.go (11)
x/evm/msg.go (1)
Addrs(537-540)x/evm/statedb/statedb.go (1)
StateDB(45-85)x/evm/embeds/embeds.go (2)
SmartContract_ERC20MinterWithMetadataUpdates(71-74)SmartContract_WNIBI(96-99)x/evm/const.go (2)
EVM_MODULE_ADDRESS(106-106)WeiToNative(136-139)x/evm/keeper/erc20.go (1)
Erc20GasLimitExecute(30-30)x/evm/events.pb.go (6)
EventConvertEvmToCoin(528-534)EventConvertEvmToCoin(538-538)EventConvertEvmToCoin(539-541)EventTxLog(128-131)EventTxLog(135-135)EventTxLog(136-138)x/evm/keeper/evm_state.go (1)
EvmState(24-47)eth/eip55.go (1)
EIP55Addr(17-19)x/evm/keeper/msg_server.go (1)
ParseWeiAsMultipleOfMicronibi(435-463)x/evm/errors.go (1)
ErrCanonicalWnibi(54-54)app/appconst/appconst.go (1)
BondDenom(22-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests
- GitHub Check: e2e-evm
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
x/evm/keeper/msg_convert_evm_to_coin.go (2)
36-37: Fix 18→6 decimal mismatch when releasing bank coins (over-credit by 1e12).
amountis ERC20 wei (18dp). Bank coins use micro (6dp). Convert withevm.WeiToNative.- bankCoins := sdk.NewCoins(sdk.NewCoin(bankDenom, sdkmath.NewIntFromBigInt(amount))) + bankAmt := sdkmath.NewIntFromBigInt(evm.WeiToNative(amount)) + bankCoins := sdk.NewCoins(sdk.NewCoin(bankDenom, bankAmt))
168-178: Fix 18→6 decimal mismatch when minting bank coins (over-credit by 1e12).
balIncreaseis ERC20 delta in wei. Convert before mint/send.- bankCoin := sdk.NewCoin(bankDenom, sdkmath.NewIntFromBigInt(balIncrease)) + bankAmt := sdkmath.NewIntFromBigInt(evm.WeiToNative(balIncrease)) + bankCoin := sdk.NewCoin(bankDenom, bankAmt)
🧹 Nitpick comments (3)
x/evm/keeper/msg_convert_evm_to_coin.go (2)
131-151: Minor: NewEVM messageToshould target the ERC20 contract; ABI pack here is redundant.
k.ERC20().Transferpacks calldata itself and calls the ERC20 contract. SettingToto the module address and pre-packingcontractInputadds noise.- contractInput, err := embeds.SmartContract_ERC20MinterWithMetadataUpdates.ABI.Pack("transfer", evm.EVM_MODULE_ADDRESS, amount) - if err != nil { - return nil, err - } + // No need to pre-pack input; ERC20().Transfer handles ABI packing. var evmObj *vm.EVM { unusedBigInt := big.NewInt(0) evmMsg := core.Message{ From: sender.Eth, - To: &evm.EVM_MODULE_ADDRESS, + To: &erc20Addr, Nonce: k.GetAccNonce(ctx, sender.Eth), Value: unusedBigInt, GasLimit: Erc20GasLimitExecute, GasPrice: unusedBigInt, GasFeeCap: unusedBigInt, GasTipCap: unusedBigInt, - Data: contractInput, + Data: nil,
317-325: Emit tx logs and update bloom for WNIBI path to match other flows.Improves observability and consistency with coin-/ERC20-originated paths.
_ = ctx.EventManager().EmitTypedEvent(&evm.EventConvertEvmToCoin{ Sender: sender.Bech32.String(), Erc20ContractAddress: erc20.Hex(), ToAddress: toAddrBech32.String(), BankCoin: withdrawnMicronibi, SenderEthAddr: sender.Eth.Hex(), }) + // Emit EVM logs and update bloom (mirrors other paths) + if evmResp != nil { + if err := ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{Logs: evmResp.Logs}); err == nil { + k.updateBlockBloom(ctx, evmResp, uint64(k.EvmState.BlockTxIndex.GetOr(ctx, 0))) + } + }x/evm/evmmodule/genesis_test.go (1)
205-206: Comment nit: direction is ERC20 → bank coin.Clarify to avoid confusion when scanning tests.
- s.T().Log("Send fungible token coins from bank to evm") + s.T().Log("Convert ERC20 to bank coin (EVM → Coin)")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
x/evm/evmmodule/genesis_test.go(2 hunks)x/evm/evmtest/erc20.go(3 hunks)x/evm/evmtest/genesis_inject.go(1 hunks)x/evm/keeper/msg_convert_evm_to_coin.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/evm/evmtest/genesis_inject.go
- x/evm/evmtest/erc20.go
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2024-12-06T16:09:01.767Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2119
File: x/evm/keeper/bank_extension_test.go:161-165
Timestamp: 2024-12-06T16:09:01.767Z
Learning: In Go test assertions within `x/evm/keeper/bank_extension_test.go`, when comparing `uint64` values using `s.Equalf`, prefer formatting the values as decimal strings with `fmt.Sprintf("%d", value)` to avoid them being printed in hexadecimal.
Applied to files:
x/evm/keeper/msg_convert_evm_to_coin.gox/evm/evmmodule/genesis_test.go
📚 Learning: 2024-12-06T11:54:38.275Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
Applied to files:
x/evm/keeper/msg_convert_evm_to_coin.gox/evm/evmmodule/genesis_test.go
📚 Learning: 2024-10-31T18:28:26.466Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2095
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-10-31T18:28:26.466Z
Learning: When implementing the `bankkeeper.Keeper` interface in `x/evm/keeper/bank_extension.go`, methods must use non-pointer receivers to satisfy the interface requirements.
Applied to files:
x/evm/keeper/msg_convert_evm_to_coin.go
📚 Learning: 2024-10-13T05:48:51.771Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the `ApplyEvmTx` function within `x/evm/keeper/msg_server.go`, a zero value for `weiPerGas` is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.
Applied to files:
x/evm/keeper/msg_convert_evm_to_coin.go
📚 Learning: 2025-09-07T16:02:13.718Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2345
File: x/evm/keeper/msg_convert_evm_to_coin.go:311-315
Timestamp: 2025-09-07T16:02:13.718Z
Learning: WNIBI (Wrapped NIBI) does not use module account escrow like regular FunTokens. WNIBI works through direct contract interactions where users deposit/withdraw native NIBI directly to/from the WNIBI contract. The fund transfers use SendCoins between user accounts, not SendCoinsFromModuleToAccount like regular FunToken conversions.
Applied to files:
x/evm/keeper/msg_convert_evm_to_coin.go
📚 Learning: 2025-01-17T17:32:27.341Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns only `*vm.EVM` without an error value.
Applied to files:
x/evm/evmmodule/genesis_test.go
📚 Learning: 2025-01-17T17:32:27.341Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns `(*vm.EVM, *statedb.StateDB)` without an error value. The second return value is often discarded when only the EVM object is needed.
Applied to files:
x/evm/evmmodule/genesis_test.go
📚 Learning: 2025-01-17T17:32:33.412Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns only an EVM object and does not return an error. The method is used in test files to create a new EVM instance for testing purposes.
Applied to files:
x/evm/evmmodule/genesis_test.go
📚 Learning: 2025-01-17T17:32:33.412Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns both an EVM object and a StateDB object with the signature `(*vm.EVM, *statedb.StateDB)`. The method is used in test files to create new EVM and StateDB instances for testing purposes.
Applied to files:
x/evm/evmmodule/genesis_test.go
🧬 Code graph analysis (2)
x/evm/keeper/msg_convert_evm_to_coin.go (10)
x/evm/msg.go (1)
Addrs(537-540)x/evm/statedb/statedb.go (1)
StateDB(45-85)x/evm/embeds/embeds.go (2)
SmartContract_ERC20MinterWithMetadataUpdates(71-74)SmartContract_WNIBI(96-99)x/evm/const.go (2)
EVM_MODULE_ADDRESS(106-106)WeiToNative(136-139)x/evm/keeper/erc20.go (1)
Erc20GasLimitExecute(30-30)x/evm/events.pb.go (6)
EventConvertEvmToCoin(528-534)EventConvertEvmToCoin(538-538)EventConvertEvmToCoin(539-541)EventTxLog(128-131)EventTxLog(135-135)EventTxLog(136-138)eth/eip55.go (1)
EIP55Addr(17-19)x/evm/keeper/msg_server.go (1)
ParseWeiAsMultipleOfMicronibi(435-463)x/evm/errors.go (1)
ErrCanonicalWnibi(54-54)app/appconst/appconst.go (1)
BondDenom(22-22)
x/evm/evmmodule/genesis_test.go (6)
x/evm/evmtest/test_deps.go (2)
NewTestDeps(36-47)TestDeps(28-34)x/evm/evm.pb.go (3)
FunToken(32-41)FunToken(45-45)FunToken(46-48)x/evm/evmtest/erc20.go (1)
FunTokenBalanceAssert(154-160)x/evm/evmtest/tx.go (1)
DeployContract(58-100)x/evm/tx.pb.go (6)
MsgCreateFunToken(480-487)MsgCreateFunToken(491-491)MsgCreateFunToken(492-494)MsgConvertEvmToCoin(675-693)MsgConvertEvmToCoin(697-697)MsgConvertEvmToCoin(698-700)x/evm/evmmodule/genesis.go (2)
ExportGenesis(86-129)InitGenesis(20-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: lint
- GitHub Check: e2e-evm
- GitHub Check: integration-tests
- GitHub Check: proto-gen-rs
- GitHub Check: build
- GitHub Check: unit-tests
🔇 Additional comments (1)
x/evm/keeper/msg_convert_evm_to_coin.go (1)
269-273: Good: balance check uses truncated withdrawWei.Prevents false rejections when
amountisn’t a multiple of micronibi.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
x/evm/evmmodule/genesis_test.go (1)
84-90: Fix unit mismatch: Bank balance must be in micro (6dp), not ERC-20 wei (18dp).Use the converter so the assertion matches Bank units.
evmtest.FunTokenBalanceAssert{ Account: toUserC, FunToken: ft, - BalanceBank: amountToSendC, + BalanceBank: evm.WeiToNative(amountToSendC), BalanceERC20: big.NewInt(0), Description: "C receives amountToSendC as Bank Coins", }.Assert(s.T(), deps, evmObj)x/evm/keeper/msg_convert_coin_to_evm.go (3)
52-58: StateDB lifecycle: restore previous pointer and only commit if we created it.Currently you (a) never set k.Bank.StateDB when creating a new StateDB, (b) always nil it on defer (clearing a previously set pointer), and (c) always commit regardless of who created it. This can clobber an existing StateDB and cause unintended commits.
Apply this pattern (restore previous, set only when created, commit only if created):
- stateDB := k.Bank.StateDB - if stateDB == nil { - stateDB = k.NewStateDB(ctx, k.TxConfig(ctx, gethcommon.Hash{})) - } - defer func() { - k.Bank.StateDB = nil - }() + prevStateDB := k.Bank.StateDB + stateDB := prevStateDB + createdState := false + if stateDB == nil { + stateDB = k.NewStateDB(ctx, k.TxConfig(ctx, gethcommon.Hash{})) + k.Bank.StateDB = stateDB + createdState = true + } + defer func() { + // Restore prior pointer regardless of our path. + k.Bank.StateDB = prevStateDB + }() @@ - if err := stateDB.Commit(); err != nil { - return nil, fmt.Errorf("failed to commit stateDB: %w", err) - } + if createdState { + if err := stateDB.Commit(); err != nil { + return nil, fmt.Errorf("failed to commit stateDB: %w", err) + } + }Also applies to: 160-164
216-223: StateDB lifecycle: same issue as above in convertCoinToEvmBornCoin.Restore previous pointer and only commit if we created the StateDB.
- stateDB := k.Bank.StateDB - if stateDB == nil { - stateDB = k.NewStateDB(ctx, txConfig) - } - defer func() { - k.Bank.StateDB = nil - }() + prevStateDB := k.Bank.StateDB + stateDB := prevStateDB + createdState := false + if stateDB == nil { + stateDB = k.NewStateDB(ctx, txConfig) + k.Bank.StateDB = stateDB + createdState = true + } + defer func() { k.Bank.StateDB = prevStateDB }() @@ - if err = stateDB.Commit(); err != nil { + if createdState { + if err = stateDB.Commit(); err != nil { return nil, sdkioerrors.Wrap(err, "failed to commit stateDB") - } + } + }Also applies to: 244-246
276-283: StateDB lifecycle: same issue in convertCoinToEvmBornERC20.Restore previous pointer and only commit if we created it.
- stateDB := k.Bank.StateDB - if stateDB == nil { - stateDB = k.NewStateDB(ctx, k.TxConfig(ctx, gethcommon.Hash{})) - } - defer func() { - k.Bank.StateDB = nil - }() + prevStateDB := k.Bank.StateDB + stateDB := prevStateDB + createdState := false + if stateDB == nil { + stateDB = k.NewStateDB(ctx, k.TxConfig(ctx, gethcommon.Hash{})) + k.Bank.StateDB = stateDB + createdState = true + } + defer func() { k.Bank.StateDB = prevStateDB }() @@ - if err := stateDB.Commit(); err != nil { - return nil, sdkioerrors.Wrap(err, "failed to commit stateDB") - } + if createdState { + if err := stateDB.Commit(); err != nil { + return nil, sdkioerrors.Wrap(err, "failed to commit stateDB") + } + }Also applies to: 348-350
🧹 Nitpick comments (10)
x/tokenfactory/cli/tx.go (1)
412-416: Prefer %q over %+q for user-facing file paths
%+qescapes non-ASCII into\uXXXX, which can reduce readability in CLI errors. Unless you explicitly want escaped Unicode, stick to%q(or%s) for clearer output and consistency with common Go CLI ergonomics.- return md, fmt.Errorf("read file %+q: %w", path, err) + return md, fmt.Errorf("read file %q: %w", path, err) - return md, fmt.Errorf("unmarshal %+q as bank.Metadata: %w", path, err) + return md, fmt.Errorf("unmarshal %q as bank.Metadata: %w", path, err)x/evm/evmmodule/genesis_test.go (6)
188-197: Avoid Int64 casts with big.Int; use big arithmetic for A+B.Prevents precision/overflow pitfalls and reads clearer.
{ evmObj, _ := deps.NewEVM() assertBalsOfAB(deps, evmObj) + sentToAB := new(big.Int).Add( + new(big.Int).Set(amountToSendA), + new(big.Int).Set(amountToSendB), + ) evmtest.FunTokenBalanceAssert{ Account: deps.Sender.EthAddr, FunToken: ft, BalanceBank: big.NewInt(0), - BalanceERC20: new(big.Int).Sub( - totalSupply, - big.NewInt(amountToSendA.Int64()+amountToSendB.Int64()), - ), + BalanceERC20: new(big.Int).Sub(new(big.Int).Set(totalSupply), sentToAB), Description: "Sender has total supply minus A and B", }.Assert(s.T(), deps, evmObj)
99-107: Avoid Int64 casts with big.Int; use big arithmetic for A+B+C.Same rationale as above.
- evmtest.FunTokenBalanceAssert{ + sentToABC := new(big.Int).Add( + new(big.Int).Add( + new(big.Int).Set(amountToSendA), + new(big.Int).Set(amountToSendB), + ), + new(big.Int).Set(amountToSendC), + ) + evmtest.FunTokenBalanceAssert{ Account: depsSender.EthAddr, FunToken: ft, BalanceBank: big.NewInt(0), - BalanceERC20: new(big.Int).Sub( - totalSupply, - big.NewInt(amountToSendA.Int64()+amountToSendB.Int64()+amountToSendC.Int64()), - ), + BalanceERC20: new(big.Int).Sub(new(big.Int).Set(totalSupply), sentToABC), Description: "Sender should lose amountToSendC", }.Assert(s.T(), deps, evmObj)
150-150: Typo in log message (“er20”).Minor clarity fix.
- s.T().Logf("Create FunToken from the er20 %s", erc20Addr) + s.T().Logf("Create FunToken from the erc20 %s", erc20Addr)
206-206: Incorrect direction in log message.This flow is ERC-20 → Bank via MsgConvertEvmToCoin.
- s.T().Log("Send fungible token coins from bank to evm") + s.T().Log("Convert ERC-20 to Bank coins (MsgConvertEvmToCoin)")
226-233: Variable name typo (“Gensis”).Rename for readability.
- bankGensisState := deps.App.BankKeeper.ExportGenesis(deps.Ctx) + bankGenesisState := deps.App.BankKeeper.ExportGenesis(deps.Ctx) @@ - deps.App.BankKeeper.InitGenesis(deps.Ctx, bankGensisState) + deps.App.BankKeeper.InitGenesis(deps.Ctx, bankGenesisState)
239-244: Minor: standardize EIP55 string compare.Compare String() to String() (or Hex to Hex) on both sides to avoid format drift.
- s.Equal(ft.Erc20Addr.Hex(), funTokens[0].Erc20Addr.String()) + s.Equal(ft.Erc20Addr.String(), funTokens[0].Erc20Addr.String())x/evm/keeper/msg_convert_coin_to_evm.go (3)
145-148: Fix user-facing error message (direction is wrong).This path transfers WNIBI to the recipient; it does not convert WNIBI→NIBI.
- return resp, fmt.Errorf("failed to convert WNIBI to NIBI: %w", err) + return resp, fmt.Errorf("failed to transfer WNIBI to recipient: %w", err)- err = fmt.Errorf("failed to convert WNIBI to NIBI: VmError: %s", evmResp.VmError) + err = fmt.Errorf("failed to transfer WNIBI to recipient: VmError: %s", evmResp.VmError)
239-243: Include VM error details for failed mint.Adding VmError greatly improves diagnosability.
- if evmResp.Failed() { - return nil, - fmt.Errorf("failed to mint erc-20 tokens of contract %s", erc20Addr.String()) - } + if evmResp.Failed() { + return nil, fmt.Errorf("failed to mint ERC-20 tokens of contract %s: VmError: %s", erc20Addr.String(), evmResp.VmError) + }
295-301: Fix comment typos/encoding."ΓåÆ" artifacts and "ERc20" → "ERC20".
- // 3 | In the FunToken ERC20 → BC conversion process that preceded this + // 3 | In the FunToken ERC20 -> Bank Coin conversion process that preceded this @@ - // on the sum of the FunToken's bank and ERC20 supply, we burn the coins here - // in the BC → ERC20 conversion. + // on the sum of the FunToken's bank and ERC20 supply, we burn the coins here + // in the Bank Coin -> ERC20 conversion. @@ - // This should never fail due to the EVM account lacking ERc20 fund because + // This should never fail due to the EVM account lacking ERC20 funds becauseAlso applies to: 303-306
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
x/evm/evmmodule/genesis_test.go(2 hunks)x/evm/keeper/msg_convert_coin_to_evm.go(1 hunks)x/evm/keeper/msg_convert_evm_to_coin.go(1 hunks)x/evm/msg.go(7 hunks)x/tokenfactory/cli/tx.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/evm/msg.go
- x/evm/keeper/msg_convert_evm_to_coin.go
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-01-17T17:32:27.341Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns only `*vm.EVM` without an error value.
Applied to files:
x/evm/evmmodule/genesis_test.go
📚 Learning: 2025-01-17T17:32:27.341Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns `(*vm.EVM, *statedb.StateDB)` without an error value. The second return value is often discarded when only the EVM object is needed.
Applied to files:
x/evm/evmmodule/genesis_test.go
📚 Learning: 2025-01-17T17:32:33.412Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns only an EVM object and does not return an error. The method is used in test files to create a new EVM instance for testing purposes.
Applied to files:
x/evm/evmmodule/genesis_test.go
📚 Learning: 2025-01-17T17:32:33.412Z
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns both an EVM object and a StateDB object with the signature `(*vm.EVM, *statedb.StateDB)`. The method is used in test files to create new EVM and StateDB instances for testing purposes.
Applied to files:
x/evm/evmmodule/genesis_test.go
📚 Learning: 2024-12-06T16:09:01.767Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2119
File: x/evm/keeper/bank_extension_test.go:161-165
Timestamp: 2024-12-06T16:09:01.767Z
Learning: In Go test assertions within `x/evm/keeper/bank_extension_test.go`, when comparing `uint64` values using `s.Equalf`, prefer formatting the values as decimal strings with `fmt.Sprintf("%d", value)` to avoid them being printed in hexadecimal.
Applied to files:
x/evm/evmmodule/genesis_test.go
📚 Learning: 2024-12-06T11:54:38.275Z
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
Applied to files:
x/evm/evmmodule/genesis_test.go
🧬 Code graph analysis (2)
x/evm/evmmodule/genesis_test.go (6)
x/evm/evmtest/test_deps.go (2)
NewTestDeps(36-47)TestDeps(28-34)x/evm/evm.pb.go (3)
FunToken(32-41)FunToken(45-45)FunToken(46-48)x/evm/evmtest/erc20.go (1)
FunTokenBalanceAssert(154-160)x/evm/evmtest/tx.go (1)
DeployContract(58-100)x/evm/tx.pb.go (6)
MsgCreateFunToken(480-487)MsgCreateFunToken(491-491)MsgCreateFunToken(492-494)MsgConvertEvmToCoin(675-693)MsgConvertEvmToCoin(697-697)MsgConvertEvmToCoin(698-700)x/evm/evmmodule/genesis.go (2)
ExportGenesis(86-129)InitGenesis(20-83)
x/evm/keeper/msg_convert_coin_to_evm.go (9)
x/evm/tx.pb.go (6)
MsgConvertCoinToEvm(582-589)MsgConvertCoinToEvm(593-593)MsgConvertCoinToEvm(594-596)MsgConvertCoinToEvmResponse(638-639)MsgConvertCoinToEvmResponse(643-643)MsgConvertCoinToEvmResponse(644-646)eth/eth_account.go (1)
NibiruAddrToEthAddr(19-21)x/evm/statedb/statedb.go (1)
StateDB(45-85)x/evm/statedb/config.go (1)
TxConfig(15-20)x/evm/errors.go (1)
ErrCanonicalWnibi(54-54)x/evm/const.go (2)
NativeToWei(123-126)EVM_MODULE_ADDRESS(106-106)x/evm/embeds/embeds.go (2)
SmartContract_WNIBI(96-99)SmartContract_ERC20MinterWithMetadataUpdates(71-74)x/evm/keeper/erc20.go (1)
Erc20GasLimitExecute(30-30)x/evm/events.pb.go (6)
EventConvertCoinToEvm(291-296)EventConvertCoinToEvm(300-300)EventConvertCoinToEvm(301-303)EventTxLog(128-131)EventTxLog(135-135)EventTxLog(136-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: integration-tests
- GitHub Check: e2e-evm
- GitHub Check: lint
- GitHub Check: proto-gen-rs
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (1)
x/evm/evmmodule/genesis_test.go (1)
79-83: Good: recreate EVM instance before assertions.Prevents stale state after (re)genesis or keeper resets.
| // isTx: value to use for commit in any EVM calls | ||
| isTx = true | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make the conversion atomic: avoid early commit in Step 1.
Passing commit=true to CallContractWithInput for the deposit can persist the WNIBI mint even if the subsequent transfer fails, breaking atomicity.
Proposed change: keep both deposit and transfer in the same StateDB transaction and commit once at the end.
- // isTx: value to use for commit in any EVM calls
- isTx = true
+ // isTx: do NOT commit mid-flow; commit once at the end for atomicity
+ isTx = false
@@
- evmResp, err := k.CallContractWithInput(
+ evmResp, err := k.CallContractWithInput(
ctx,
evmObj,
senderEthAddr, /* fromAcc */
&erc20.Address, /* contract */
- isTx, /* commit */
+ isTx, /* commit */
contractInput,
Erc20GasLimitExecute,
depositWei,
)To confirm Transfer does not commit on its own, please inspect its implementation:
Also applies to: 105-114
🏁 Script executed:
#!/bin/bash
# Verify whether ERC20.Transfer commits internally
rg -nP -C3 'func\s+\(.*\)\s+Transfer\(' x/evm
rg -nP -C3 'CallContractWithInput\s*\(' x/evm/keeper/erc20.goLength of output: 1709
Defer state commit until after both mint and transfer
In x/evm/keeper/msg_convert_coin_to_evm.go (around lines 39–41 and 105–114), change the deposit call’s commit flag to false so the WNIBI mint and subsequent ERC-20 transfer run in a single StateDB transaction.
🤖 Prompt for AI Agents
In x/evm/keeper/msg_convert_coin_to_evm.go around lines 39–41 and 105–114, the
deposit call currently sets the commit flag to true which causes an immediate
state commit; change that deposit call’s commit parameter to false so the WNIBI
mint and the subsequent ERC‑20 transfer execute in the same StateDB transaction,
and ensure the state is committed only after both operations complete (i.e., set
commit = false on deposit and keep the existing commit/flush logic after
mint+transfer).
feat(evm): add "eth.evm.v1.MsgConvertEvmToCoin" tx for ERC20 to bank coin conversions with a non-Ethereum transaction
This pull request extends the EVM module with full support for converting between ERC20 tokens and Cosmos bank coins. It introduces new message types, CLI commands, and keeper logic to cover both directions of conversion, including special-case handling for NIBI via WNIBI.
WNIBI
1. ERC20 → Bank Coin Conversion
New Messages and API
MsgConvertEvmToCoinandMsgConvertEvmToCoinResponseinproto/eth/evm/v1/tx.proto.ConvertEvmToCoin.EventConvertEvmToCoininproto/eth/evm/v1/events.prototo record conversions.CLI
convert-evm-to-coincommand inx/evm/cli/tx.gowith full test coverage incli_test.go.Keeper Logic
Implemented
ConvertEvmToCoininmsg_server.go.Two execution paths:
Emits typed events and transaction logs.
Added validation to prevent conversion without a registered FunToken mapping.
2. Bank Coin → ERC20 Conversion with WNIBI Support
WNIBI Special Case (NIBI Wrapping)
convertCoinToEvmForWNIBIto handle wrapping NIBI into its ERC20 equivalent, WNIBI.General FunToken Flow
Existing
ConvertCoinToEvmlogic extended to distinguish between:Upgrade Handling
v2_7_0upgrade to set canonical WNIBI in the EVM params for mainnet.MAINNET_WNIBI_ADDR,MAINNET_STNIBI_ADDR) defined inappconst.3. Testing and Validation
Unit Tests
msg_convert_evm_to_coin_test.gowith full scenarios: happy paths, insufficient funds, invalid addresses, and event checks.msg_convert_coin_to_evm_test.gowith WNIBI coverage and race condition scenarios.BalanceAssertNIBI,FunTokenBalanceAssert).Genesis and Params Validation
Params.CanonicalWnibicannot be the zero address.Minor Fixes and Updates
MsgConvertCoinToEvmvalidation.EIP55Addr.Equalhelper method.CallContractWithInputto accept an optionalweiValue.Summary
With these changes, Nibiru supports:
MsgConvertEvmToCoin.MsgConvertCoinToEvm.