feat!: bank precompile #2860
feat!: bank precompile #2860
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe pull request introduces a precompiled contract for managing banking operations within a blockchain environment, allowing for the deposit and withdrawal of ZRC20 tokens as Cosmos bank tokens. It includes updates to various files, implementing functions for balance inquiries, deposits, and withdrawals. Additionally, the changes enhance error handling and logging capabilities, ensuring robust interactions with the banking contract. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
fbac
force-pushed
the
feat/bank-precompile
branch
3 times, most recently
from
004c3af
to
3eecbbf
Compare
fbac
force-pushed
the
feat/bank-precompile
branch
3 times, most recently
from
977f122
to
55aaf96
Compare
precompiles/bank/method_deposit.go
Outdated
| // function balanceOf(address account) public view virtual override returns (uint256) | ||
| argsBalanceOf := []interface{}{caller} | ||
|
|
||
| resBalanceOf, err := c.CallContract(ctx, ZRC20ABI, ZRC20Addr, "balanceOf", argsBalanceOf) |
There was a problem hiding this comment.
This is the balanceOf for the ERC20 zrc20 balance in the contract address?
This checks is valid but we should think about going further: this is an invariant that should never occur, if it occur it mean something wrong is with the blockchain like ability to mint the ZRC20 in the module
In the future we should look into adding an invariant: https://docs.cosmos.network/v0.46/building-modules/invariants.html
erc20Balance >= minted ZRC20 Cosmos coin
There was a problem hiding this comment.
Exactly. We check the caller has enough balance of ZRC20 to spend, and then we check the bank has an allowance enough to handle the amount of tokens the caller wants to mint.
Taking a look at invariants.
fbac
force-pushed
the
feat/bank-precompile
branch
3 times, most recently
from
4099e86
to
ab771a8
Compare
fbac
force-pushed
the
feat/bank-precompile
branch
from
6afd879
to
c1836a3
Compare
fbac
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
skosito and
swift1337
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (6)
precompiles/precompiles_test.go (1)
28-28: LGTM!The addition of the
sdkk.BankKeeperparameter to theStatefulContractsfunction aligns with the PR objective of implementing the bank precompile. The test assertions adequately verify the expected behavior of the function.Consider adding the following test cases to further enhance the test coverage:
- Test the behavior when the
EnabledStatefulContractsvariable is empty or partially populated.- Verify that the returned contracts have the expected functionality by invoking their methods and asserting the results.
precompiles/bank/method_withdraw.go (1)
17-138: Thewithdrawfunction implementation looks good overall.The function correctly validates the input arguments, checks the caller's balance, and executes the token transfer from the contract to the caller. It interacts with the bank module to send and burn the withdrawn coins and logs the transaction.
A few suggestions for improvement:
- Consider consolidating the error handling code into a separate function to reduce repetition and improve readability.
- Add more detailed logging statements to capture the input parameters and the results of each operation for better traceability.
e2e/e2etests/test_precompiles_bank.go (1)
15-119: Consider enhancing the test coverage and readability.While the test function is well-structured and covers the core functionality of the bank precompile, consider the following suggestions to further improve the test coverage and readability:
- Add test cases for edge cases, such as attempting to withdraw more than the available balance or depositing/withdrawing zero amounts.
- Consider extracting the repeated code blocks for checking event emissions and balance updates into separate helper functions to improve readability and maintainability.
- Add comments to explain the purpose of each test step and the expected outcomes to enhance the test's clarity.
These enhancements will make the test more comprehensive and easier to understand and maintain.
precompiles/bank/bank.go (1)
117-174: LGTM with a minor suggestion!The
Runfunction correctly serves as the entrypoint of the precompiled contract by:
- Retrieving the method by ID from the ABI.
- Unpacking the method arguments from the input.
- Executing the corresponding logic based on the method name.
- Handling read-only mode restrictions for
DepositMethodNameandWithdrawMethodName.- Executing the
deposit,withdraw, orbalanceOffunction within a native action and returning the result or error.- Returning an error for invalid methods.
Please remove the debug lines (e.g.,
fmt.Println("DEBUG: ...")) before merging to production.contrib/localnet/scripts/start-zetacored.sh (1)
273-274: Consider reducing the token amount for the new genesis account.Funding the account with 100,000,000,000,000,000,000,000,000 azeta tokens may be excessive for testing purposes. It's advisable to use a smaller amount that still allows for comprehensive testing of the bank precompile functionality without potentially skewing results or causing unexpected behavior due to the large balance.
app/app.go (1)
Line range hint
1069-1078: Consider refactoring the code for improved readability and conciseness.While the logic is correct, the code can be refactored to improve readability and conciseness. Here's a suggested refactoring:
for addr, enabled := range precompiles.EnabledStatefulContracts { if addr != bankprecompile.ContractAddress && enabled { blockList[addr.String()] = true } }This refactoring combines the conditional check and the addition to the
blockListinto a single statement, making the code more concise and easier to read.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (34)
- Dockerfile-localnet (2 hunks)
- app/app.go (3 hunks)
- changelog.md (1 hunks)
- cmd/zetae2e/config/localnet.yml (1 hunks)
- cmd/zetae2e/local/local.go (1 hunks)
- codecov.yml (1 hunks)
- contrib/localnet/orchestrator/start-zetae2e.sh (1 hunks)
- contrib/localnet/scripts/start-zetacored.sh (1 hunks)
- e2e/e2etests/e2etests.go (2 hunks)
- e2e/e2etests/test_precompiles_bank.go (1 hunks)
- e2e/e2etests/test_rate_limiter.go (2 hunks)
- e2e/e2etests/test_stress_btc_deposit.go (1 hunks)
- e2e/e2etests/test_stress_btc_withdraw.go (1 hunks)
- e2e/e2etests/test_stress_eth_deposit.go (1 hunks)
- e2e/e2etests/test_stress_eth_withdraw.go (1 hunks)
- precompiles/bank/IBank.abi (1 hunks)
- precompiles/bank/IBank.go (1 hunks)
- precompiles/bank/IBank.json (1 hunks)
- precompiles/bank/IBank.sol (1 hunks)
- precompiles/bank/bank.go (1 hunks)
- precompiles/bank/bindings.go (1 hunks)
- precompiles/bank/coin.go (1 hunks)
- precompiles/bank/const.go (1 hunks)
- precompiles/bank/logs.go (1 hunks)
- precompiles/bank/method_balance_of.go (1 hunks)
- precompiles/bank/method_deposit.go (1 hunks)
- precompiles/bank/method_withdraw.go (1 hunks)
- precompiles/logs/logs.go (2 hunks)
- precompiles/precompiles.go (3 hunks)
- precompiles/precompiles_test.go (1 hunks)
- precompiles/staking/logs.go (3 hunks)
- precompiles/types/errors.go (2 hunks)
- precompiles/types/errors_test.go (2 hunks)
- precompiles/types/types.go (3 hunks)
Files skipped from review due to trivial changes (9)
- Dockerfile-localnet
- codecov.yml
- contrib/localnet/orchestrator/start-zetae2e.sh
- e2e/e2etests/test_rate_limiter.go
- e2e/e2etests/test_stress_btc_deposit.go
- e2e/e2etests/test_stress_btc_withdraw.go
- e2e/e2etests/test_stress_eth_deposit.go
- e2e/e2etests/test_stress_eth_withdraw.go
- precompiles/bank/IBank.go
Additional context used
Path-based instructions (20)
precompiles/bank/bindings.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/bank/const.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/precompiles_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/bank/coin.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/logs/logs.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/types/errors.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/bank/method_balance_of.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/types/errors_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/bank/logs.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/staking/logs.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/precompiles.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/types/types.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/bank/method_withdraw.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/bank/method_deposit.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_precompiles_bank.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/bank/bank.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetae2e/local/local.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.contrib/localnet/scripts/start-zetacored.sh (1)
Pattern
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.e2e/e2etests/e2etests.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.app/app.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (73)
precompiles/bank/bindings.go (2)
1-3: Excellent use ofgo:generatedirectives for automating Go bindings generation.The
go:generatedirectives provide a clean and efficient way to automate the generation of Go bindings for theIBankSolidity contract. The directives are well-structured and follow a logical sequence of compilation, ABI extraction, and code generation.Assuming the generated files (
IBank.json,IBank.abi,IBank.go) are correct, this code segment is approved.
7-7: Clarify the purpose of the_ Contractvariable declaration.The variable declaration
var _ Contractis likely used to ensure that thebankpackage implements theContractinterface. This is a common Go idiom to perform compile-time interface checks.However, the
Contracttype is not defined in the provided code. Please clarify the purpose of this declaration and ensure that thebankpackage actually implements the required methods of theContractinterface.precompiles/bank/const.go (7)
3-5: LGTM!The constant is well-defined, follows the naming convention, and has a clear comment describing its purpose.
7-8: LGTM!The constant is well-defined, follows the naming convention, and has a clear comment describing its purpose.
9-9: LGTM!The constant is well-defined, follows the naming convention, and uses an underscore to improve readability of the integer literal.
11-11: LGTM!The constant is well-defined, follows the naming convention, and has a clear purpose.
12-12: LGTM!The constant is well-defined, follows the naming convention, and uses an underscore to improve readability of the integer literal.
15-15: LGTM!The constant is well-defined, follows the naming convention, and has a clear purpose.
16-16: LGTM!The constant is well-defined, follows the naming convention, and has an appropriate gas cost relative to the other methods.
precompiles/bank/coin.go (2)
15-17: The function is well-implemented and follows best practices.The
ZRC20ToCosmosDenomfunction correctly converts a ZRC20 address to a Cosmos coin denomination by prefixing it with "zevm/". The implementation is concise and easy to understand.
21-44: The function is well-implemented and handles error cases effectively.The
createCoinSetfunction correctly creates asdk.Coinsobject from a token denomination and amount. It performs necessary validations to ensure the created coin and coin set are valid. If any validation fails, it returns appropriate error messages using theptypes.ErrInvalidCointype.The function is well-documented with comments explaining the purpose and logic, making it easy to understand and maintain.
precompiles/logs/logs.go (3)
11-14: LGTM!The
Argumentstruct is well-defined, with appropriately named and typed fields. Usinginterface{}for theValuefield provides flexibility to store any data type. The struct definition is clean and follows Go conventions.
44-68: LGTM!The
PackArgumentsfunction is well-implemented and follows best practices:
- The function signature is clear and follows Go conventions.
- It handles an arbitrary number of arguments using a slice of
Argumentstructs.- It constructs ABI types and packs the values using the
abipackage fromgo-ethereum.- Error handling is implemented correctly, returning any errors encountered during the process.
- The function is well-documented with comments explaining its purpose and usage.
The implementation is clean, efficient, and provides a flexible way to pack arguments for logs.
50-60: LGTM!The loop implementation is correct and follows best practices:
- It correctly iterates over each
Argumentin theargsslice.- Constructing the ABI type using
abi.NewTypeis the appropriate approach.- Error handling is implemented correctly, returning any errors encountered during the process.
- Appending the constructed ABI type to
typesand theValuefield totoPackis necessary for packing the arguments later.- The loop is concise and follows Go conventions.
The loop effectively prepares the arguments for packing in a clean and efficient manner.
precompiles/types/errors.go (5)
8-11: Excellent addition of theReasonfield toErrInvalidAddr.The inclusion of the
Reasonfield enhances the error message, providing more context about the invalid address. This change is beneficial for debugging and user feedback.
42-50: Well-designedErrInvalidCointype andError()method.The introduction of the
ErrInvalidCointype improves the granularity of error reporting for invalid coin scenarios. The struct fields (Got,Negative,Nil) capture relevant information about the invalid coin, and the correspondingError()method formats a detailed error message. This implementation enhances debugging capabilities and follows Go best practices.
52-58: Well-designedErrInvalidAmounttype andError()method.The introduction of the
ErrInvalidAmounttype improves the specificity of error reporting for invalid token amount scenarios. TheGotfield captures the invalid amount, and the correspondingError()method formats a clear error message. This implementation enhances error handling and follows Go best practices.
60-67: Well-designedErrInsufficientBalancetype andError()method.The introduction of the
ErrInsufficientBalancetype improves the specificity of error reporting for insufficient balance scenarios. TheRequestedandGotfields capture the relevant balance information, and the correspondingError()method formats a detailed error message. This implementation enhances error handling and follows Go best practices.
81-88: Well-designedErrUnexpectedtype andError()method.The introduction of the
ErrUnexpectedtype provides a structured way to handle and report unexpected errors. TheWhenandGotfields capture the context and details of the unexpected error, and the correspondingError()method formats a detailed error message. This implementation enhances error handling, aids in debugging, and follows Go best practices.precompiles/bank/method_balance_of.go (4)
11-47: ThebalanceOffunction is well-structured and follows best practices.The function follows a clear and logical flow, with appropriate error handling. The use of the
unpackBalanceOfArgshelper function improves code readability and maintainability. The function correctly validates the number of arguments, handles invalid addresses, retrieves the balance using thebankKeeper, checks for the validity of the returned coin, and packs the balance amount into the output format specified by the ABI method.
16-21: The error handling in thebalanceOffunction is comprehensive and informative.The function correctly checks for the number of arguments and returns an appropriate error. It handles errors returned by helper functions (
unpackBalanceOfArgsandgetCosmosAddress) and checks for the validity of the retrieved coin balance, returning an appropriate error. The error messages provide sufficient information about the cause of the error, making it easier to diagnose and fix issues.Also applies to: 25-27, 31-33, 38-44
49-65: TheunpackBalanceOfArgsfunction is a clean and type-safe way to extract addresses from input arguments.The function provides a clean and type-safe way to extract the ZRC20 token address and user address from the input arguments. The use of type assertions ensures that the arguments are of the correct type (
common.Address). The function returns an appropriate error if the type assertions fail, providing information about the invalid address. This helper function improves the readability and maintainability of thebalanceOffunction by encapsulating the argument unpacking logic.
1-65: The code is well-structured, follows best practices, and is easy to understand.The code is organized into two functions:
balanceOfandunpackBalanceOfArgs, with a clear separation of concerns. ThebalanceOffunction is a method of theContractstruct, whileunpackBalanceOfArgsis a standalone helper function. The code imports necessary packages and uses appropriate naming conventions for variables and functions. The inclusion of comments explaining the purpose and functionality of the functions enhances code clarity and understandability. The code follows best practices for Go programming, such as using short variable names and returning errors as the last return value. Overall, the code is well-structured and easy to maintain.precompiles/types/errors_test.go (5)
7-8: LGTM!The addition of the
Reasonfield to theErrInvalidAddrstruct enhances the error message by providing more context about the invalid address. The test function has been updated accordingly to verify the new error message format.Also applies to: 11-11
51-62: LGTM!The new
Test_ErrInvalidCoinfunction thoroughly tests theErrInvalidCoinerror type by creating an instance with various fields and verifying the generated error message. This ensures that the error handling logic for invalid coins is robust and provides clear feedback.
64-73: LGTM!The new
Test_ErrInvalidAmountfunction tests theErrInvalidAmounterror type by creating an instance with an invalid amount and verifying the generated error message. This ensures that the error handling logic for invalid token amounts is accurate and provides clear feedback.
75-85: LGTM!The new
Test_ErrUnexpectedfunction tests theErrUnexpectederror type by creating an instance with a specific scenario and unexpected value, then verifying the generated error message. This ensures that the error handling logic for unexpected scenarios is robust and provides clear feedback.
87-97: LGTM!The new
Test_ErrInsufficientBalancefunction tests theErrInsufficientBalanceerror type by creating an instance with a requested amount and current balance, then verifying the generated error message. This ensures that the error handling logic for insufficient balance scenarios is accurate and provides clear feedback.precompiles/staking/logs.go (3)
40-42: LGTM!The changes to pack the amount argument using
logs.PackArgumentsare correct and follow the appropriate data type for representing token amounts.
72-74: LGTM!The changes to pack the amount argument using
logs.PackArgumentsare correct and follow the appropriate data type for representing token amounts.
115-117: LGTM!The changes to pack the amount argument using
logs.PackArgumentsare correct and follow the appropriate data type for representing token amounts.precompiles/precompiles.go (3)
7-7: LGTM!The import statement is correct, and the package alias is clear and descriptive.
14-14: LGTM!The import statement is correct, and the package name is clear and descriptive.
Line range hint
26-68: LGTM!The code segment follows the existing code structure and patterns. The addition of the
bank.ContractAddressto theEnabledStatefulContractsmap, thebankKeeperparameter to theStatefulContractsfunction, and thebankContractfunction to theprecompiledContractsslice are all correct and consistent with the existing code.precompiles/types/types.go (4)
36-43: LGTM!The
ContractCallerinterface provides a clear contract for invoking contract methods on behalf of a precompiled contract. TheCallContractmethod signature is well-defined, including all necessary parameters and return values for flexibility and error handling.
47-47: LGTM!Embedding the
ContractCallerinterface in theBaseContractinterface ensures that any struct implementingBaseContractmust also implement theCallContractmethod. This change enforces the capability to call other contract methods for any contract implementingBaseContract.
69-117: LGTM!The implementation of the
CallContractmethod in thebaseContractstruct correctly handles the invocation of the contract method through thefungibleKeeper. It takes care of important details such as:
- Setting
noEthereumTxEventto true to avoid failures with multiple Ethereum transactions.- Using the precompiled contract's address as the
fromaddress to ensure the call is made on behalf of the precompiled contract.- Handling errors from the EVM call and unpacking the returned data.
- Returning the unpacked return values and any encountered error.
The code is well-structured and follows best practices for error handling and contract interactions.
Line range hint
1-117: LGTM!The Go code in this file follows the principles of clean code, expressiveness, and performance:
- The code has a clear and logical structure, making it easy to understand and navigate.
- The naming conventions for variables, functions, and types are appropriate and descriptive.
- The code includes comments explaining important aspects of the implementation, enhancing readability and maintainability.
- The interfaces and structs are well-defined, providing a clear contract for interaction.
- Error handling is done appropriately, with errors being returned for further handling by the caller.
- There are no apparent performance issues in the code.
Overall, the code is well-written, expressive, and adheres to best practices for Go development.
precompiles/bank/IBank.abi (3)
2-38: TheDepositevent is well-defined.The event captures all the necessary information about a deposit transaction, including the depositor's address, the ZRC20 token address, the corresponding Cosmos token identifier, the user's Cosmos address, and the deposited amount. Indexing the first three parameters enables efficient querying of the event logs.
39-74: TheWithdrawevent is well-defined.The event captures all the necessary information about a withdrawal transaction, including the withdrawer's address, the ZRC20 token address, the corresponding Cosmos token identifier, the user's Cosmos address, and the withdrawn amount. Indexing the first three parameters enables efficient querying of the event logs.
76-147: ThebalanceOf,deposit, andwithdrawfunctions are well-defined.
- The
balanceOffunction is appropriately marked as a view function, as it doesn't modify the contract state and can be called without incurring any gas costs.- The
depositandwithdrawfunctions are correctly marked as nonpayable, as they are intended to handle ZRC20 tokens and not Ether.- The function names are clear, concise, and accurately describe their purpose.
- The parameter names are descriptive and follow a consistent naming convention, enhancing code readability.
- The return values are appropriately typed and named, providing clarity about the expected output.
Overall, the function definitions adhere to best practices and contribute to a well-structured and maintainable contract interface.
precompiles/bank/IBank.sol (6)
16-71: The interface is well-defined and follows best practices.The
IBankinterface is properly structured, follows Solidity naming conventions, and is thoroughly documented with NatSpec comments. Defining the interface at a fixed address is a good practice for precompiled contracts.
17-29: TheDepositevent is well-defined and properly indexed.The
Depositevent is thoroughly documented with NatSpec comments, and its parameters are well-named and follow the naming conventions. The event parameters are properly indexed, allowing for efficient filtering of events.
31-43: TheWithdrawevent is well-defined and properly indexed.The
Withdrawevent is thoroughly documented with NatSpec comments, and its parameters are well-named and follow the naming conventions. The event parameters are properly indexed, allowing for efficient filtering of events.
45-52: Thedepositfunction is well-defined and properly documented.The
depositfunction is thoroughly documented with NatSpec comments, and its parameters are well-named and follow the naming conventions. The function return value is properly documented, indicating the success status of the deposit operation.
54-61: Thewithdrawfunction is well-defined and properly documented.The
withdrawfunction is thoroughly documented with NatSpec comments, and its parameters are well-named and follow the naming conventions. The function return value is properly documented, indicating the success status of the withdrawal operation.
63-70: ThebalanceOffunction is well-defined and properly documented.The
balanceOffunction is thoroughly documented with NatSpec comments, and its parameters are well-named and follow the naming conventions. The function return value is properly documented, indicating the balance of the Cosmos token for the specified ZRC20 token and user.precompiles/bank/IBank.json (5)
3-39: TheDepositevent is well-defined.The event captures all the essential details required for tracking deposit transactions, including the depositor address, ZRC20 token address, Cosmos token, Cosmos address, and the deposited amount. The indexing of relevant fields enables efficient querying of deposit events.
40-76: TheWithdrawevent is well-defined.The event captures all the essential details required for tracking withdraw transactions, including the withdrawer address, ZRC20 token address, Cosmos token, Cosmos address, and the withdrawn amount. The indexing of relevant fields enables efficient querying of withdraw events.
77-100: ThebalanceOffunction is well-defined.The function accepts the necessary parameters, namely the ZRC20 token address and the user address, to query the balance of a specific user for a given ZRC20 token. The return type
uint256is suitable for representing the balance value. Theviewmodifier correctly indicates that the function does not modify the contract state.
101-124: Thedepositfunction is well-defined.The function accepts the necessary parameters, namely the ZRC20 token address and the deposit amount, to facilitate the transfer of ZRC20 tokens from the user to the contract. The return type
boolis suitable for indicating the success or failure of the deposit operation. Thenonpayablemodifier correctly specifies that the function does not accept Ether.
125-148: Thewithdrawfunction is well-defined.The function accepts the necessary parameters, namely the ZRC20 token address and the withdraw amount, to enable users to withdraw their ZRC20 tokens from the contract. The return type
boolis suitable for indicating the success or failure of the withdraw operation. Thenonpayablemodifier correctly specifies that the function does not accept Ether.precompiles/bank/method_withdraw.go (1)
140-156: TheunpackWithdrawArgsfunction is implemented correctly.The function provides a clean way to unpack and validate the input arguments for the withdraw function. It encapsulates the argument validation logic and returns appropriate errors, improving code readability and error handling.
precompiles/bank/method_deposit.go (2)
16-159: Excellent implementation of the deposit functionality.The
depositfunction is well-structured and follows the Check-Effects-Interactions pattern for safe execution. It correctly validates the input arguments, checks the caller's balance and allowance, and handles the creation, minting, and transfer of the corresponding Cosmos coin. The logging of deposit transactions is a valuable addition for auditing purposes.Overall, this function effectively integrates ZRC20 tokens into the Cosmos ecosystem and provides a secure and reliable deposit mechanism.
161-177: Well-implemented argument unpacking and validation.The
unpackDepositArgsfunction is a clean and reusable helper function that unpacks and validates the arguments for thedepositfunction. It ensures that the ZRC20 token address is of typecommon.Addressand the deposit amount is a non-negative and non-zero*big.Int.The function handles invalid arguments gracefully by returning appropriate errors, preventing potential issues in the
depositfunction.This is a solid implementation that enhances the robustness and maintainability of the codebase.
cmd/zetae2e/config/localnet.yml (1)
64-67: Verify if the emptyprivate_keyfield foruser_bank_precompileis intentional.The addition of the new account
user_bank_precompileunder theadditional_accountssection looks good. However, please confirm if the emptyprivate_keyfield is intentional. If this account is expected to perform transactions that require signing, then an empty private key might lead to issues.e2e/e2etests/test_precompiles_bank.go (3)
15-23: Appropriate temporary gas limit increase for precompiled functions.The temporary increase of the gas limit is necessary to accommodate the gas-intensive operations of the precompiled functions. The code correctly restores the original gas limit using a
deferstatement.
25-86: Thorough testing of the deposit functionality.The test performs a comprehensive validation of the deposit functionality:
- It creates a bank contract caller and deposits an initial amount of WZETA tokens.
- It approves the bank contract to spend a portion of the WZETA tokens and performs a deposit.
- It checks the deposit event emission and verifies the updated balances of the spender and the bank contract.
The assertions used are appropriate and cover the expected behavior and state changes.
88-118: Comprehensive testing of the withdraw functionality.The test thoroughly validates the withdraw functionality:
- It performs a withdrawal of a portion of the deposited WZETA tokens.
- It checks the withdraw event emission and verifies the updated balances of the spender and the bank contract.
The assertions used are suitable and cover the expected behavior and state changes.
precompiles/bank/bank.go (6)
62-83: LGTM!The constructor function is well-structured and follows best practices by:
- Accepting necessary dependencies as parameters.
- Instantiating the ZRC20 ABI only once to optimize performance.
- Returning a new
Contractinstance with the provided dependencies.
86-88: LGTM!The
Addressfunction correctly returns theContractAddressconstant and implements thePrecompiledContractinterface.
91-93: LGTM!The
Abifunction correctly returns theABIconstant and implements thePrecompiledContractinterface.
97-113: LGTM!The
RequiredGasfunction correctly calculates the required gas for a given input by:
- Extracting the method ID from the input.
- Calculating a base cost using the input size and the gas config.
- Adjusting the base cost for read-only methods.
- Adding the predefined gas required for the method, if found in the
GasRequiredByMethodmap.- Returning 0 if the method is not found in the map.
180-187: LGTM!The
getEVMCallerAddressfunction correctly returns the caller address by:
- Assigning the
contract.CallerAddressto thecallervariable.- Checking if the call was made through a contract by comparing the
contract.CallerAddresswith theevm.Origin.- Assigning the
evm.Originto thecallervariable if the call was made through a contract.- Returning the
calleraddress.
191-208: LGTM!The
getCosmosAddressfunction correctly returns the counterpart cosmos address of the given ethereum address by:
- Converting the given ethereum address to a cosmos address.
- Checking if the cosmos address is empty and returning an error if true.
- Checking if the cosmos address is blocked by the bank keeper and returning an error if true.
- Returning the cosmos address if it passes the checks.
cmd/zetae2e/local/local.go (1)
324-324: LGTM: New test case added consistently.The addition of the
e2etests.TestPrecompilesBankNametest case to the list of precompiled contract tests follows the existing pattern and naming convention. The test case is correctly added only if theskipPrecompilesflag is not set.e2e/e2etests/e2etests.go (2)
162-163: LGTM!The added constants follow the existing naming convention and have descriptive values that clearly indicate the purpose of the tests.
882-887: LGTM!The new end-to-end test is added consistently with the existing tests in the
AllE2ETestsslice. The test name and description align with the added constants, and the empty argument definition array is appropriate if no additional arguments are required. Assuming the associated test functionTestPrecompilesBankcontains the actual test implementation, the changes look good.app/app.go (4)
96-96: LGTM!The import statement for the
bankprecompilepackage is correct and necessary for integrating the bank precompile functionality.
574-574: LGTM!Passing
app.BankKeeperto theprecompiles.StatefulContractsfunction is necessary for the bank precompile to interact with the bank module.
1069-1073: LGTM!The conditional check to skip adding the bank precompile address to the blocked addresses list is correct and necessary to allow the bank precompile to receive funds.
Line range hint
1069-1078: LGTM!The logic to add enabled stateful precompiles to the blocked addresses list, while skipping the bank precompile, is correct and necessary. This ensures that stateful precompiles are marked as non-payable by the bank keeper, except for the bank precompile which needs to receive funds.
changelog.md (1)
12-12: Changelog entry looks good.The new feature for the bank precompiled contract has been documented correctly in the changelog, following established conventions.
fbac
force-pushed
the
feat/bank-precompile
branch
from
39a8437
to
f1e5b3a
Compare
fbac
force-pushed
the
feat/bank-precompile
branch
from
4232a98
to
05d0bfb
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2860 +/- ##
===========================================
- Coverage 66.85% 65.88% -0.98%
===========================================
Files 378 384 +6
Lines 21103 21487 +384
===========================================
+ Hits 14108 14156 +48
- Misses 6330 6663 +333
- Partials 665 668 +3
|
| @@ -61,6 +61,10 @@ additional_accounts: | |||
| bech32_address: "zeta1nry9yeg6njhjrp2ctppa8558vqxal9fxk69zxg" | |||
| evm_address: "0x98c852651A9CAF2185585843d3D287600Ddf9526" | |||
| private_key: "bf9456c679bb5a952a9a137fcfc920e0413efdb97c36de1e57455763084230cb" | |||
| user_bank_precompile: | |||
| bech32_address: "zeta1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqr84sgapz" | |||
| evm_address: "0x0000000000000000000000000000000000000067" | |||
There was a problem hiding this comment.
all other users used here are external accounts used to interact with contracts, not the precompile itself, is this intentional?
There was a problem hiding this comment.
It's intentional. The bank precompile has to call external contracts, each one of these calls is a transaction originated by the bank precompile and the fungible keeper will try every time to retrieve the sequence/nonce for the account.
If the account doesn't exist and sequence/nonce is not retrieved, the transaction can't be created.
The cleanest way to make sure the account exists every time is to introduce it from genesis.
There was a problem hiding this comment.
How do we create the account on live network? I think we should avoid having this account in the config here as it doesn't represent a user, it is not clean to me.
| // getEVMCallerAddress returns the caller address. | ||
| // Usually the caller is the contract.CallerAddress, which is the address of the contract that called the precompiled contract. | ||
| // If contract.CallerAddress != evm.Origin is true, it means the call was made through a contract, | ||
| // on which case there is a need to set the caller to the evm.Origin. |
There was a problem hiding this comment.
could you explain why is this set like this? if some contract is calling function and you replace caller by origin, you need to authorize all operations from origin to contract, or this is not needed?
There was a problem hiding this comment.
The current design expects the original caller approves the bank contract to spend X tokens on their behalf:
- EOA runs
ZRC20.approve(bankAddress, X), so bank can spend X ZRC20 on their behalf. - EOA calls
bank.deposit(ZRC20Addr, X)orcontract.depositToBank(ZRC20Addr, X).
Open to discussions here.
| } | ||
|
|
||
| // Get the correct caller address. | ||
| caller, err := getEVMCallerAddress(evm, contract) |
There was a problem hiding this comment.
what happens if acc -> smart contract and that smart contract calls deposit maliciously?
in that case caller is replaced with origin here and it is executed without checking that original caller authorized smart contract to deposit for account? or that is handled somehow, if that is the case i think it would be clearer to explain in comment a bit
There was a problem hiding this comment.
If an EOA calls some function in a malicious contract that tries to deposit in bank:
- the bank will try to transfer funds from the original caller (either if it's contract.Caller or evm.Origin)
- if the bank doesn't have allowance from the original EOA, the call fails.
- if the bank does have allowance, the new coins are minted and ZRC20 is locked in bank
- new cosmos coins are minted and sent to the original EOA cosmos address, not to the contracts.
| "github.com/zeta-chain/node/precompiles/bank" | ||
| ) | ||
|
|
||
| func TestPrecompilesBank(r *runner.E2ERunner, args []string) { |
There was a problem hiding this comment.
This test should be refactored to reset values at the beginning or use relative values, so that it still works if we run it for a second time( for the TSS migration test )
| "github.com/zeta-chain/node/x/fungible/types" | ||
| ) | ||
|
|
||
| // From IBank.sol: function deposit(address zrc20, uint256 amount) external returns (bool success); |
There was a problem hiding this comment.
might be a good idea to expand on the comment here to explain a bit more about what the function does, as this is a very important section of code
Same for withdraw
| @@ -61,6 +61,10 @@ additional_accounts: | |||
| bech32_address: "zeta1nry9yeg6njhjrp2ctppa8558vqxal9fxk69zxg" | |||
| evm_address: "0x98c852651A9CAF2185585843d3D287600Ddf9526" | |||
| private_key: "bf9456c679bb5a952a9a137fcfc920e0413efdb97c36de1e57455763084230cb" | |||
| user_bank_precompile: | |||
| bech32_address: "zeta1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqr84sgapz" | |||
| evm_address: "0x0000000000000000000000000000000000000067" | |||
There was a problem hiding this comment.
How do we create the account on live network? I think we should avoid having this account in the config here as it doesn't represent a user, it is not clean to me.
| return res, nil | ||
|
|
||
| case BalanceOfMethodName: | ||
| fmt.Println("DEBUG: bank.Run(): BalanceOfMethodName") |
There was a problem hiding this comment.
Can we remove these println now?
| if method.Name == DepositMethodName { | ||
| fmt.Println("DEBUG: bank.Run(): DepositMethodName: ExecuteNativeAction c.deposit()") | ||
| res, err = c.deposit(ctx, evm, contract, method, args) | ||
| } else { |
There was a problem hiding this comment.
Can we use if method.Name == WithdrawMethodName here for extra safety?
| // ZEVM cosmos coins prefix. | ||
| ZEVMDenom = "zevm/" |
There was a problem hiding this comment.
Can we maybe use zrc20 as the prefix?
|
|
||
| // Caller has to have enough cosmos coin balance to withdraw the requested amount. | ||
| coin := c.bankKeeper.GetBalance(ctx, fromAddr, ZRC20ToCosmosDenom(zrc20Addr)) | ||
| if coin.Amount.LT(math.NewIntFromBigInt(amount)) { |
There was a problem hiding this comment.
I think we should check coin.Amount is not nil first
| bankContract, err := bank.NewIBank(bank.ContractAddress, r.ZEVMClient) | ||
| require.NoError(r, err, "Failed to create bank contract caller") | ||
|
|
||
| // Deposit and approve 50 WZETA for the test. |
There was a problem hiding this comment.
WZETA is not a ZRC20, we should test with ZRC20ETH and ZRC20ERC20
| ) | ||
|
|
||
| // From IBank.sol: function deposit(address zrc20, uint256 amount) external returns (bool success); | ||
| func (c *Contract) deposit( |
There was a problem hiding this comment.
We want to restrict the functionality to only whitelisted ZRC20 for now.
We can check existence with GetForeignCoins of Fungible module, we should also not do the action if the coin is paused
| bankContract, err := bank.NewIBank(bank.ContractAddress, r.ZEVMClient) | ||
| require.NoError(r, err, "Failed to create bank contract caller") | ||
|
|
||
| // Deposit and approve 50 WZETA for the test. |
There was a problem hiding this comment.
The deposit should actually not be possible with WZETA so this can be used to test we can't deposit it
Description
Closes #2791.
Implements a bank precompile, with the following functions:
WIP
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores