feat(evm): Deduct a fee during the creation of a FunToken mapping. Implemented by deductCreateFunTokenFee inside of the eth.evm.v1.MsgCreateFunToken transaction.
#1963
Conversation
WalkthroughIn a significant update to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Unique-Divine
added
type: enhancement
x: evm
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1963 +/- ##
==========================================
+ Coverage 65.64% 65.91% +0.27%
==========================================
Files 257 258 +1
Lines 16434 16397 -37
==========================================
+ Hits 10788 10808 +20
+ Misses 4841 4780 -61
- Partials 805 809 +4
|
There was a problem hiding this comment.
Unused code here, so we delete it
x/evm/embeds/ERC20Minter.sol
Outdated
There was a problem hiding this comment.
Fixed some of the doc comments in this file, as some were not accurate
x/evm/errors.go
Outdated
There was a problem hiding this comment.
| // computeCommitGasLimit: If the transition is meant to mutate state, this | ||
| // function computes an appopriates gas limit for the call with "contractInput" | ||
| // bytes against the given contract address. | ||
| // | ||
| // ℹ️ This creates a cached context for gas estimation, which is essential | ||
| // because state transitions can occur outside of the EVM that are triggered | ||
| // by Ethereum transactions, like in the case of precompiled contract or | ||
| // custom EVM hook that runs after tx execution. Gas estimation in that case | ||
| // could mutate the "ctx" object and result in falty resulting state, so we | ||
| // must cache here to avoid this issue. |
There was a problem hiding this comment.
Big explainer added here since the reasoning behind the cachedCtx is nuanced
| func AssertERC20BalanceEqual( | ||
| t *testing.T, | ||
| deps *TestDeps, | ||
| deps TestDeps, | ||
| contract, account gethcommon.Address, | ||
| balance *big.Int, | ||
| ) { |
There was a problem hiding this comment.
This should take a non-pointer receiver because it's not modifying state and only reading it
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
x/evm/evm.pb.gois excluded by!**/*.pb.go
Files selected for processing (18)
- CHANGELOG.md (1 hunks)
- proto/eth/evm/v1/evm.proto (2 hunks)
- proto/eth/evm/v1/tx.proto (1 hunks)
- x/evm/embeds/ERC20Minter.sol (2 hunks)
- x/evm/embeds/ERC20MinterCompiled.json (1 hunks)
- x/evm/errors.go (1 hunks)
- x/evm/evmtest/erc20.go (3 hunks)
- x/evm/evmtest/eth_test.go (1 hunks)
- x/evm/keeper/erc20.go (5 hunks)
- x/evm/keeper/erc20_test.go (11 hunks)
- x/evm/keeper/funtoken_from_coin.go (1 hunks)
- x/evm/keeper/funtoken_from_erc20.go (1 hunks)
- x/evm/keeper/msg_server.go (2 hunks)
- x/evm/keeper/precompiles.go (3 hunks)
- x/evm/params.go (3 hunks)
- x/evm/precompile/funtoken.go (1 hunks)
- x/evm/precompile/funtoken_test.go (4 hunks)
- x/evm/precompile/precompile.go (1 hunks)
Files skipped from review due to trivial changes (5)
- proto/eth/evm/v1/tx.proto
- x/evm/embeds/ERC20MinterCompiled.json
- x/evm/errors.go
- x/evm/evmtest/eth_test.go
- x/evm/keeper/funtoken_from_coin.go
Additional context used
GitHub Check: codecov/patch
x/evm/keeper/funtoken_from_erc20.go
[warning] 111-111: x/evm/keeper/funtoken_from_erc20.go#L111
Added line #L111 was not covered by tests
[warning] 118-118: x/evm/keeper/funtoken_from_erc20.go#L118
Added line #L118 was not covered by tests
[warning] 123-123: x/evm/keeper/funtoken_from_erc20.go#L123
Added line #L123 was not covered by tests
[warning] 145-145: x/evm/keeper/funtoken_from_erc20.go#L145
Added line #L145 was not covered by testsx/evm/keeper/erc20.go
[warning] 108-108: x/evm/keeper/erc20.go#L108
Added line #L108 was not covered by tests
[warning] 140-140: x/evm/keeper/erc20.go#L140
Added line #L140 was not covered by testsx/evm/keeper/msg_server.go
[warning] 514-514: x/evm/keeper/msg_server.go#L514
Added line #L514 was not covered by tests
[warning] 550-550: x/evm/keeper/msg_server.go#L550
Added line #L550 was not covered by tests
[warning] 553-553: x/evm/keeper/msg_server.go#L553
Added line #L553 was not covered by tests
Additional comments not posted (39)
x/evm/embeds/ERC20Minter.sol (1)
59-67: Rename function and update comments for clarity.The function name has been changed to
burnFromAuthorityto better reflect its purpose. The comments have been updated to describe the use case accurately.x/evm/evmtest/erc20.go (3)
12-12: Update import path fortestapppackage.The import path for the
testapppackage has been updated togithub.com/NibiruChain/nibiru/x/common/testutil/testapp.
27-27: Remove pointer fromTestDepsparameter.The
AssertERC20BalanceEqualfunction signature has been updated to remove the pointer from theTestDepsparameter, aligning with the changes in the test dependencies.
59-67: Fund account for fee before creating FunToken.The function now includes logic to fund the sender account with the necessary fee before creating a FunToken. This ensures that the operation can proceed without running into insufficient funds issues.
x/evm/precompile/precompile.go (1)
99-99: Rename function toOnRunStart.The function name has been updated from
OnStarttoOnRunStartto better reflect its purpose and control flow.x/evm/precompile/funtoken_test.go (5)
89-90: LGTM! Ensure correct usage ofAssertERC20BalanceEqual.The function signature change is correctly applied here.
111-112: LGTM! Ensure correct usage ofAssertERC20BalanceEqual.The function signature change is correctly applied here.
122-123: LGTM! Ensure correct usage ofAssertERC20BalanceEqual.The function signature change is correctly applied here.
140-141: LGTM! Ensure correct usage ofAssertERC20BalanceEqual.The function signature change is correctly applied here.
146-147: LGTM! Ensure correct usage ofAssertERC20BalanceEqual.The function signature change is correctly applied here.
x/evm/keeper/funtoken_from_erc20.go (4)
19-55: LGTM! Ensure error handling and metadata retrieval logic.The function correctly handles metadata retrieval and error handling.
57-61: LGTM! The type definition is appropriate.The
ERC20Metadatatype is well-defined.
63-71: LGTM! The type definitions are appropriate.The types for unpacking Solidity data are well-defined.
73-161: LGTM! Ensure error handling and validation logic.The function correctly handles the creation of
FunTokenand performs necessary checks and validations.Missing test coverage for some lines.
Several lines are not covered by tests. Ensure these lines are tested to maintain code quality.
Would you like me to help generate the missing tests or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 111-111: x/evm/keeper/funtoken_from_erc20.go#L111
Added line #L111 was not covered by tests
[warning] 118-118: x/evm/keeper/funtoken_from_erc20.go#L118
Added line #L118 was not covered by tests
[warning] 123-123: x/evm/keeper/funtoken_from_erc20.go#L123
Added line #L123 was not covered by tests
[warning] 145-145: x/evm/keeper/funtoken_from_erc20.go#L145
Added line #L145 was not covered by testsx/evm/params.go (3)
Line range hint
10-19:
LGTM! The imports are necessary and correctly used.The new imports for
cosmossdk.io/mathandgithub.com/NibiruChain/nibiru/app/appconstare appropriate.
25-25: LGTM! The constant value is appropriately updated.The value of
DefaultEVMDenomis updated toappconst.BondDenom, which is consistent with the rest of the codebase.
65-65: LGTM! The new parameter is appropriate.The
CreateFuntokenFeeparameter is correctly added to the default parameters.x/evm/precompile/funtoken.go (4)
62-62: LGTM! The function call update is appropriate.The function call is correctly updated from
OnStarttoOnRunStart.
Line range hint
104-104:
LGTM! Ensure error handling and validation logic.The function correctly implements the
bankSendmethod and handles necessary validations and error handling.Also applies to: 108-108
Line range hint
191-191:
LGTM! The function logic is appropriate.The function correctly constructs arguments for the
bankSendmethod.
Line range hint
199-199:
LGTM! The function logic is appropriate.The function correctly validates the argument types for the
bankSendmethod.proto/eth/evm/v1/evm.proto (2)
35-39: Clarify comments forenable_createandenable_call.The comments for
enable_createandenable_callfields are clear and provide necessary information about their functionality.
54-60: Add thecreate_funtoken_feefield to theParamsmessage.The addition of the
create_funtoken_feefield in theParamsmessage is well-documented and follows the protobuf syntax correctly. It includes appropriategogoprotooptions for custom type and non-nullability. However, it is essential to ensure that this field is handled correctly in the codebase where theParamsmessage is used.x/evm/keeper/erc20.go (7)
21-27: Add theERC20function.The
ERC20function returns anerc20Callsstruct with a reference to the keeper and the ERC20 contract ABI. This is a good approach to encapsulate ERC20-related functionality. Ensure that the returned struct is used correctly in the codebase.
31-33: Define theerc20Callsstruct.The
erc20Callsstruct encapsulates the keeper and the ERC20 ABI. This struct is well-defined and provides a clear way to group ERC20-related methods.
36-61: Implement theMintmethod.The
Mintmethod implements the ERC20 mint function. The method packs the input arguments using the ABI and calls the contract with the packed input. This implementation is correct, but ensure that theCallContractWithInputfunction handles errors and edge cases appropriately.
63-83: Implement theTransfermethod.The
Transfermethod implements the ERC20 transfer function. The method follows a similar pattern to theMintmethod, packing input arguments and calling the contract. This implementation is correct and consistent with theMintmethod.
85-92: Implement theBalanceOfmethod.The
BalanceOfmethod retrieves the balance of an ERC20 token for a specific account. This method is well-implemented and uses theLoadERC20BigIntfunction to load the balance.
94-111: Implement theBurnmethod.The
Burnmethod implements the ERC20 burn function. The method follows a similar pattern to theMintandTransfermethods, packing input arguments and calling the contract. This implementation is correct and consistent with the other methods.Tools
GitHub Check: codecov/patch
[warning] 108-108: x/evm/keeper/erc20.go#L108
Added line #L108 was not covered by tests
Line range hint
223-245:
Improve gas limit computation incomputeCommitGasLimit.The
computeCommitGasLimitfunction computes an appropriate gas limit for the contract call. The function creates a cached context for gas estimation, which is essential to avoid mutating thectxobject. This implementation is correct and necessary for accurate gas estimation.x/evm/keeper/erc20_test.go (5)
78-86: Add test case for funding accounts for fees before operations.The test case ensures that the sender is funded with the necessary fee before creating a FunToken. This is a good practice to ensure that the fee deduction logic is tested.
104-112: Add test case for creating a FunToken with an existing ERC20.The test case ensures that the sender is funded with the necessary fee before creating a FunToken with an existing ERC20. This is a good practice to ensure that the fee deduction logic is tested.
188-196: Add test case for creating a FunToken from a bank coin.The test case ensures that the sender is funded with the necessary fee before creating a FunToken from a bank coin. This is a good practice to ensure that the fee deduction logic is tested.
233-240: Add test case for creating a FunToken from a bank coin that is already registered.The test case ensures that the sender is funded with the necessary fee before attempting to create a FunToken from a bank coin that is already registered. This is a good practice to ensure that the fee deduction logic is tested.
304-312: Add test case for funding accounts for fees before sending FunToken to EVM.The test case ensures that the sender is funded with the necessary fee before sending a FunToken to the EVM. This is a good practice to ensure that the fee deduction logic is tested.
x/evm/keeper/msg_server.go (3)
510-515: Add fee deduction logic inCreateFunToken.The
CreateFunTokenfunction now includes logic to deduct a fee upon registration. Ensure that this fee deduction is correctly handled and that appropriate error handling is in place.Tools
GitHub Check: codecov/patch
[warning] 514-514: x/evm/keeper/msg_server.go#L514
Added line #L514 was not covered by tests
544-556: Implement thedeductCreateFunTokenFeefunction.The
deductCreateFunTokenFeefunction handles the deduction and burning of the fee for creating a FunToken. The implementation is correct and includes proper error handling. However, ensure that this function is thoroughly tested to cover all edge cases.Tools
GitHub Check: codecov/patch
[warning] 550-550: x/evm/keeper/msg_server.go#L550
Added line #L550 was not covered by tests
[warning] 553-553: x/evm/keeper/msg_server.go#L553
Added line #L553 was not covered by tests
558-561: Implement theFeeForCreateFunTokenfunction.The
FeeForCreateFunTokenfunction retrieves the fee for creating a FunToken from the module parameters. This implementation is correct and necessary for determining the fee amount.CHANGELOG.md (1)
85-85: Changelog entry looks good.The new entry documents the addition of the
deductCreateFunTokenFeemethod in theeth.evm.v1.MsgCreateFunTokentransaction. The format and placement are correct.
| // | ||
| // TODO: refactor(evm/keeper/precompiles): Use ordered map as the underlying | ||
| // struct to remove the need for iterating over k.precompiles in so many | ||
| // different ways. The set could also be tracked as well to make it ea |
There was a problem hiding this comment.
Refactor suggestion: Use ordered map.
A TODO comment suggests refactoring to use an ordered map as the underlying structure to remove the need for iterating over k.precompiles in multiple ways. This can enhance performance and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments not posted (1)
CHANGELOG.md (1)
85-85: Ensure the changelog entry follows the correct format.The changelog entry is correctly placed under the "State Machine Breaking" section with a reference to the PR. Ensure that the entry provides a clear and concise description of the change.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- CHANGELOG.md (1 hunks)
- x/common/testutil/testnetwork/network.go (1 hunks)
- x/common/testutil/testnetwork/validator_node.go (1 hunks)
- x/evm/errors.go (1 hunks)
- x/evm/keeper/funtoken_from_coin.go (1 hunks)
- x/evm/keeper/msg_server.go (2 hunks)
- x/evm/params.go (3 hunks)
- x/evm/precompile/funtoken.go (1 hunks)
- x/evm/precompile/funtoken_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (6)
- CHANGELOG.md
- x/evm/keeper/funtoken_from_coin.go
- x/evm/keeper/msg_server.go
- x/evm/params.go
- x/evm/precompile/funtoken.go
- x/evm/precompile/funtoken_test.go
Additional comments not posted (3)
x/evm/errors.go (1)
15-18: LGTM!The
ErrOwnableconstant is well-defined and follows the convention of other error constants in the file.x/common/testutil/testnetwork/validator_node.go (1)
133-136: LGTM!The addition of error logging for gRPC web server closure improves the robustness of the shutdown process.
x/common/testutil/testnetwork/network.go (1)
604-604: LGTM!Increasing the
maxRetriesvalue from 5 to 20 ensures that the cleanup process has adequate attempts to succeed, reducing the likelihood of address-in-use errors.
Unique-Divine
force-pushed
the
ud/funtoken-fee
branch
from
85e20fd
to
6f49104
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- x/evm/keeper/funtoken_from_erc20.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/evm/keeper/funtoken_from_erc20.go
Additional comments not posted (1)
CHANGELOG.md (1)
86-86: Change documented correctly.The addition of the
deductCreateFunTokenFeemethod is well-documented under the "State Machine Breaking" section for the next mainnet version, with a link to PR #1963.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/evm/keeper/erc20_test.go (11 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/evm/keeper/erc20_test.go
Purpose / Abstract
Part of #1898 and #1836
Summary by CodeRabbit
New Features
FunTokenmappings in the Ethereum Virtual Machine (EVM).Bug Fixes
TestCreateFunTokenFromERC20,TestCreateFunTokenFromCoin, andTestSendFunTokenToEvm.Refactor
MINTER_ROLErole with an "owner" role inERC20Minter.OnStarttoOnRunStartfor clarity.Documentation