Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(evm): keeper collections and grpc query impls for EthAccount, NibiruAccount #1873

Merged
merged 15 commits into from
May 15, 2024

Conversation

Unique-Divine
Copy link
Member

@Unique-Divine Unique-Divine commented May 14, 2024

  1. Part of epic(evm): Nibiru EVM tracking ticket #1836
  2. Deletes unused protobuf files
  3. Upgrades to collections v0.5 to use MapTransient and ItemTransient
  4. Implements the NibiruAccount and EthAccount queries
  5. Further updates the module wiring.

Copy link
Contributor

coderabbitai bot commented May 14, 2024

Walkthrough

The changes in the NibiruChain/nibiru project encompass significant updates related to Ethereum integration, including new keeper collections and gRPC query implementations for Ethereum and Nibiru accounts. Additionally, the codebase has undergone a refactor to improve readability and maintainability, such as consolidating validation logic and renaming various functions and variables for clarity. The codec handling has been standardized across multiple files, switching from Marshaler to Codec.

Changes

File/Path Change Summary
CHANGELOG.md Added features related to Ethereum (eth) and Ethereum Virtual Machine (evm).
app/ante.go Refactored NewAnteHandler to use ValidateAndClean method for validation.
app/ante/fixed_gas_test.go Updated expected gas value in TestOraclePostPriceTransactionsHaveFixedPrice.
app/ante/testutil_test.go Modified NewNibiruTestApp to use Codec instead of Marshaler.
app/app.go Changed appCodec to use Codec instead of Marshaler.
app/appconst/appconst.go Renamed Version() to RuntimeVersion().
app/codec/codec.go Renamed Marshaler field in EncodingConfig struct to Codec.
app/ibc_test.go Updated parameter to app.NewDefaultGenesisState to use Codec.
app/keepers.go Added import for eth and modified AccountKeeper initialization.
app/modules_test.go Updated to use Codec instead of Marshaler in multiple functions.
cmd/nibid/cmd/decode_base64_test.go Replaced Marshaler with Codec in TestBase64Decode.
cmd/nibid/cmd/genaccounts_test.go Replaced Marshaler with Codec in TestAddGenesisAccountCmd.
cmd/nibid/cmd/root.go Replaced Marshaler with Codec in WithCodec method call.
cmd/nibid/cmd/testnet_test.go Switched from Marshaler to Codec in multiple function calls.
eth/chain_id.go Removed errorsmod import and refactored error handling.
eth/chain_id_test.go Refactored TestParseChainID into TestParseChainID_Happy and TestParseChainID_Sad.
eth/codec.go Added constants for transaction extension protobuf type URLs.
eth/encoding/config.go Registered Amino codec with mb before registering interfaces.
eth/eth_account.go Introduced functionality related to Ethereum accounts.
eth/indexer/kv_indexer_test.go Updated encCfg declaration to use app.MakeEncodingConfig().
eth/rpc/backend/account_info.go Renamed QueryAccountRequest to QueryEthAccountRequest.
eth/rpc/backend/evm_query_client_test.go Renamed function call and request/response types.
eth/rpc/backend/mocks/evm_query_client.go Renamed functions and updated request/response types.
eth/rpc/rpcapi/web3_api.go ClientVersion now returns appconst.RuntimeVersion().
proto/eth/evm/v1/query.proto Renamed RPC methods and message types related to Ethereum account queries.
x/common/error.go Renamed ErrNilMsg() to ErrNilGrpcMsg().
x/common/testutil/cli/network.go Changed Codec field assignment to use Codec.
x/common/testutil/cli/network_test.go Modified cdc variable declaration in TestLogMnemonic.
x/common/testutil/cli/util.go Updated NewKeyring to use Codec.
x/common/testutil/genesis/genesis.go Replaced Marshaler with Codec in NewTestGenesisState.
x/common/testutil/genesis/oracle_genesis.go Replaced Marshaler with Codec in AddOracleGenesis.
x/common/testutil/genesis/sudo_genesis.go Replaced Marshaler with Codec in AddSudoGenesis.
x/common/testutil/testapp/testapp.go Switched from Marshaler to Codec for JSON operations.

In the code's realm, where bytes do dance,
Ethereum's magic takes its stance.
Keepers and queries now align,
With Codec's touch, the stars do shine.
Ante's logic, clean and bright,
Nibiru's future, ever in sight.
A rabbit's joy, in code's delight. 🐇✨


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented May 14, 2024

Codecov Report

Attention: Patch coverage is 38.83162% with 178 lines in your changes are missing coverage. Please review.

Project coverage is 67.13%. Comparing base (6e38c19) to head (5187d0f).

❗ Current head 5187d0f differs from pull request most recent head 75d3990. Consider uploading reports for the commit 75d3990 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1873      +/-   ##
==========================================
- Coverage   67.79%   67.13%   -0.67%     
==========================================
  Files         208      212       +4     
  Lines       13228    13467     +239     
==========================================
+ Hits         8968     9041      +73     
- Misses       3619     3780     +161     
- Partials      641      646       +5     
Files Coverage Δ
app/app.go 54.36% <100.00%> (ø)
app/keepers.go 99.16% <100.00%> (+<0.01%) ⬆️
cmd/nibid/cmd/root.go 77.93% <100.00%> (ø)
eth/codec.go 58.33% <ø> (ø)
eth/encoding/config.go 100.00% <100.00%> (ø)
eth/rpc/backend/account_info.go 83.03% <100.00%> (ø)
x/common/testutil/cli/network.go 77.15% <100.00%> (ø)
x/common/testutil/cli/util.go 74.32% <100.00%> (ø)
x/evm/codec.go 84.21% <ø> (ø)
x/evm/const.go 0.00% <ø> (ø)
... and 12 more

... and 1 file with indirect coverage changes

@Unique-Divine Unique-Divine changed the title feat(evm): keeper grpc query impls and collection start feat(evm): keeper collections and grpc query impls for EthAccount, NibiruAccount May 15, 2024
@Unique-Divine Unique-Divine marked this pull request as ready for review May 15, 2024 04:32
@Unique-Divine Unique-Divine requested a review from a team as a code owner May 15, 2024 04:32
@Unique-Divine Unique-Divine merged commit fac6492 into main May 15, 2024
15 of 16 checks passed
@Unique-Divine Unique-Divine deleted the ud/eth-2 branch May 15, 2024 04:34
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 24

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 6e38c19 and 75d3990.
Files ignored due to path filters (4)
  • go.mod is excluded by !**/*.mod, !**/*.mod
  • go.sum is excluded by !**/*.sum, !**/*.sum
  • x/evm/query.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • x/evm/query.pb.gw.go is excluded by !**/*.pb.gw.go, !**/*.pb.gw.go
Files selected for processing (55)
  • CHANGELOG.md (1 hunks)
  • app/ante.go (2 hunks)
  • app/ante/fixed_gas_test.go (1 hunks)
  • app/ante/testutil_test.go (1 hunks)
  • app/app.go (1 hunks)
  • app/appconst/appconst.go (1 hunks)
  • app/codec/codec.go (2 hunks)
  • app/ibc_test.go (1 hunks)
  • app/keepers.go (3 hunks)
  • app/modules_test.go (2 hunks)
  • cmd/nibid/cmd/decode_base64_test.go (1 hunks)
  • cmd/nibid/cmd/genaccounts_test.go (1 hunks)
  • cmd/nibid/cmd/root.go (1 hunks)
  • cmd/nibid/cmd/testnet_test.go (2 hunks)
  • eth/chain_id.go (3 hunks)
  • eth/chain_id_test.go (1 hunks)
  • eth/codec.go (1 hunks)
  • eth/codec_test.go (1 hunks)
  • eth/encoding/config.go (1 hunks)
  • eth/eth_account.go (1 hunks)
  • eth/indexer/kv_indexer_test.go (1 hunks)
  • eth/rpc/backend/account_info.go (1 hunks)
  • eth/rpc/backend/evm_query_client_test.go (1 hunks)
  • eth/rpc/backend/mocks/evm_query_client.go (4 hunks)
  • eth/rpc/rpcapi/web3_api.go (1 hunks)
  • proto/eth/evm/v1/query.proto (4 hunks)
  • x/common/error.go (1 hunks)
  • x/common/testutil/cli/network.go (1 hunks)
  • x/common/testutil/cli/network_test.go (1 hunks)
  • x/common/testutil/cli/util.go (1 hunks)
  • x/common/testutil/genesis/genesis.go (1 hunks)
  • x/common/testutil/genesis/oracle_genesis.go (1 hunks)
  • x/common/testutil/genesis/sudo_genesis.go (1 hunks)
  • x/common/testutil/testapp/testapp.go (3 hunks)
  • x/epochs/genesis_test.go (1 hunks)
  • x/evm/codec.go (1 hunks)
  • x/evm/const.go (2 hunks)
  • x/evm/evmmodule/genesis.go (2 hunks)
  • x/evm/evmmodule/module.go (2 hunks)
  • x/evm/evmtest/eth.go (3 hunks)
  • x/evm/keeper/evm_state.go (1 hunks)
  • x/evm/keeper/grpc_query.go (5 hunks)
  • x/evm/keeper/grpc_query_test.go (1 hunks)
  • x/evm/keeper/keeper.go (2 hunks)
  • x/evm/keeper/keeper_test.go (1 hunks)
  • x/evm/keeper/statedb.go (1 hunks)
  • x/evm/msg.go (2 hunks)
  • x/evm/query.go (1 hunks)
  • x/evm/query_test.go (1 hunks)
  • x/evm/statedb/interfaces.go (1 hunks)
  • x/genmsg/integration_test.go (2 hunks)
  • x/oracle/types/genesis_test.go (1 hunks)
  • x/oracle/types/test_utils.go (1 hunks)
  • x/sudo/cli/cli_test.go (1 hunks)
  • x/tokenfactory/keeper/msg_server.go (1 hunks)
Files not processed due to max files limit (1)
  • x/tokenfactory/keeper/wasm_test.go
Files skipped from review due to trivial changes (7)
  • app/appconst/appconst.go
  • x/common/error.go
  • x/common/testutil/cli/util.go
  • x/common/testutil/genesis/genesis.go
  • x/evm/codec.go
  • x/evm/keeper/keeper_test.go
  • x/evm/query.go
Additional comments not posted (77)
eth/rpc/rpcapi/web3_api.go (1)

21-21: Update method to return RuntimeVersion.

The change to return appconst.RuntimeVersion() is correct and aligns with the renaming in appconst.go.

x/oracle/types/genesis_test.go (1)

22-22: Update codec usage in test.

The change to use app.MakeEncodingConfig().Codec is correct and aligns with the shift from Marshaler to Codec.

x/common/testutil/genesis/oracle_genesis.go (1)

13-13: Update codec usage in AddOracleGenesis.

The change to use app.MakeEncodingConfig().Codec is correct and aligns with the shift from Marshaler to Codec.

x/common/testutil/genesis/sudo_genesis.go (1)

22-23: Update codec usage in AddSudoGenesis.

The change to use app.MakeEncodingConfig().Codec is correct and aligns with the shift from Marshaler to Codec.

app/codec/codec.go (2)

14-14: Update EncodingConfig struct to use Codec.

The change to use codec.Codec in the EncodingConfig struct is correct and aligns with the shift from Marshaler to Codec.


28-28: Update MakeEncodingConfig function to use Codec.

The change to use codec.Codec in the MakeEncodingConfig function is correct and aligns with the shift from Marshaler to Codec.

eth/encoding/config.go (1)

29-29: Update codec usage in MakeConfig.

The change to use amino.NewProtoCodec in the MakeConfig function is correct and aligns with the shift from Marshaler to Codec.

x/evm/query_test.go (1)

14-57: The test cases for nil queries are well-structured and ensure that nil requests return the appropriate error. Consider adding more test cases for other potential edge cases if necessary.

eth/chain_id.go (2)

25-28: The IsValidChainID function correctly validates the chain ID format using a regular expression. The implementation looks good.


41-55: The ParseChainID function correctly handles trimming, length checking, and parsing of the chain ID. Error handling is appropriately implemented. The implementation looks good.

x/evm/statedb/interfaces.go (2)

Line range hint 12-15: The ExtStateDB interface is well-defined and extends the existing vm.StateDB interface appropriately with the AppendJournalEntry method.


22-39: The Keeper interface is well-structured and covers essential functionalities for managing Ethereum accounts and contract storage. The implementation looks good.

x/genmsg/integration_test.go (1)

Line range hint 23-52: The TestIntegration test case is well-structured and verifies the integration of the genmsg module with the bank module. The implementation looks good.

app/modules_test.go (1)

Line range hint 34-71: The TestGenesis test case is well-structured and covers both default and custom genesis states for the staking module. Error handling is appropriately implemented. The implementation looks good.

cmd/nibid/cmd/testnet_test.go (1)

Line range hint 28-52: The Test_TestnetCmd test case is well-structured and verifies the correct execution of the testnet command. The changes from Marshaler to Codec are correctly implemented. The implementation looks good.

x/evm/const.go (1)

36-39: Ensure unique namespace values.

Verify that the namespace values are unique across the entire codebase to avoid conflicts.

eth/codec.go (1)

34-38: Ensure proper usage of constants.

Verify that the new constants TYPE_URL_WEB3_TX and TYPE_URL_DYNAMIC_FEE_TX are used correctly throughout the codebase.

Verification successful

The new constants TYPE_URL_WEB3_TX and TYPE_URL_DYNAMIC_FEE_TX are correctly used in the test file eth/codec_test.go to verify their presence and correctness in the implementation of TxExtensionOptionI.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the usage of new constants `TYPE_URL_WEB3_TX` and `TYPE_URL_DYNAMIC_FEE_TX`.

# Test: Search for the usage of the new constants. Expect: Proper usage in relevant files.
rg --type go 'TYPE_URL_WEB3_TX'
rg --type go 'TYPE_URL_DYNAMIC_FEE_TX'

Length of output: 320



Script:

#!/bin/bash
# Description: Verify the context and correctness of the usage of new constants in the test file.

# Test: Display the lines around the usage of the constants in the test file.
rg --type go -C 5 'TYPE_URL_WEB3_TX' eth/codec_test.go
rg --type go -C 5 'TYPE_URL_DYNAMIC_FEE_TX' eth/codec_test.go

Length of output: 555

eth/codec_test.go (2)

51-52: LGTM! The interface registration and implementation checks are comprehensive.


Line range hint 91-97: LGTM! The filtering logic is straightforward and effective.

eth/chain_id_test.go (2)

12-41: LGTM! The test cases for valid chain IDs are comprehensive and well-structured.


43-122: LGTM! The test cases for invalid chain IDs are extensive and ensure thorough validation.

eth/eth_account.go (3)

31-42: LGTM! The EthAccountI interface is well-defined and comprehensive.


44-84: LGTM! The EthAccount struct and its methods are correctly implemented.


21-29: LGTM! The EthAccType constants are well-defined and appropriately used.

x/evm/evmtest/eth.go (6)

27-29: LGTM! The function correctly generates a new Ethereum address.


31-38: LGTM! The function correctly generates a pair of Ethereum and Nibiru addresses.


40-42: LGTM! The function correctly converts an Ethereum address to a Nibiru address.


Line range hint 44-50: LGTM! The function correctly generates an Ethereum private key and address.


Line range hint 52-54: LGTM! The function correctly returns a new Ethereum transaction message.


Line range hint 83-101: LGTM! The function correctly converts an Ethereum transaction message to consensus engine bytes.

cmd/nibid/cmd/decode_base64_test.go (5)

34-34: LGTM! The test case correctly checks a valid base64 message.


34-34: LGTM! The test case correctly checks a valid base64 message with an additional field.


34-34: LGTM! The test case correctly checks a valid base64 message with an additional field in a different format.


34-34: LGTM! The test case correctly checks an empty base64 message.


34-34: LGTM! The test case correctly checks an invalid JSON message.

app/ante.go (2)

33-34: LGTM! The function correctly initializes the AnteHandler with the necessary decorators.


65-88: LGTM! The function correctly validates and cleans the AnteHandlerOptions.

x/common/testutil/cli/network_test.go (1)

89-89: Update to use Codec instead of Marshaler looks good.

app/ante/testutil_test.go (1)

42-42: Update to use Codec instead of Marshaler looks good.

x/epochs/genesis_test.go (1)

23-23: Update to use Codec instead of Marshaler looks good.

x/evm/keeper/evm_state.go (2)

1-99: The new types and methods for handling EVM state are well-defined and follow existing patterns. The use of collections for managing state is appropriate.


101-147: The methods for setting and getting account code and state are well-implemented and handle edge cases appropriately.

x/oracle/types/test_utils.go (1)

152-152: The addition of the SharesFromTokensTruncated method to the MockValidator struct is consistent with the existing methods and follows the expected pattern.

x/evm/keeper/grpc_query_test.go (1)

1-183: The new test cases for querying NibiruAccount and EthAccount are comprehensive and cover various scenarios. The use of helper functions and structs improves readability and maintainability.

x/evm/evmmodule/module.go (1)

96-96: Registering Ethereum-related interfaces is necessary for handling Ethereum accounts and transactions.

eth/indexer/kv_indexer_test.go (1)

45-46: Using app.MakeEncodingConfig() ensures consistency with the application's encoding configuration.

x/common/testutil/testapp/testapp.go (3)

39-39: Using encoding.Codec ensures consistency with the updated encoding configuration.


89-89: Using encoding.Codec ensures consistency with the updated encoding configuration.


124-124: Using encoding.Codec ensures consistency with the updated encoding configuration.

eth/rpc/backend/account_info.go (2)

98-98: Using QueryEthAccountRequest ensures the function uses the correct query request type for Ethereum accounts.


102-102: Using EthAccount ensures the function uses the correct response type for Ethereum accounts.

x/tokenfactory/keeper/msg_server.go (1)

16-16: Introducing a reusable error variable for nil gRPC messages improves code readability and maintainability.

app/ibc_test.go (1)

37-37: Using encodingConfig.Codec ensures consistency with the updated encoding configuration.

x/sudo/cli/cli_test.go (1)

266-266: Change aligns with the shift from Marshaler to Codec.

app/ante/fixed_gas_test.go (1)

200-200: Update reflects the new expected gas value.

cmd/nibid/cmd/root.go (1)

42-42: Change aligns with the shift from Marshaler to Codec.

x/evm/keeper/grpc_query.go (7)

9-11: Necessary imports for new gRPC query implementations.


19-29: Comment provides a clear description of the EthAccount method.


47-57: Comment provides a clear description of the NibiruAccount method.


114-116: Validation check ensures request validity.


144-149: Validation check ensures request validity.


192-196: Comment provides a clear description of the Params method.


231-243: Comment provides a clear description of the EstimateGasForEvmCallType method.

proto/eth/evm/v1/query.proto (7)

16-18: Addition of the EthAccount RPC method aligns with new gRPC query implementations.


21-23: Addition of the NibiruAccount RPC method aligns with new gRPC query implementations.


80-81: Addition of the QueryEthAccountRequest message aligns with new gRPC query implementations.


89-90: Addition of the QueryEthAccountResponse message aligns with new gRPC query implementations.


99-101: Addition of the QueryNibiruAccountRequest message aligns with new gRPC query implementations.


109-111: Addition of the QueryNibiruAccountResponse message aligns with new gRPC query implementations.


133-133: Addition of the QueryValidatorAccountResponse message aligns with new gRPC query implementations.

eth/rpc/backend/mocks/evm_query_client.go (2)

Line range hint 20-43: EthAccount method implementation looks good and follows the existing pattern for mock functions.


Line range hint 140-163: NibiruAccount method implementation looks good and follows the existing pattern for mock functions.

eth/rpc/backend/evm_query_client_test.go (1)

287-288: Update mock registration to use EthAccount.

The change to use EthAccount instead of Account in the RegisterAccount function is correct and aligns with the new naming conventions.

app/app.go (1)

133-133: Update encoding configuration to use Codec.

The change to use Codec instead of Marshaler in the NewNibiruApp function is correct and aligns with the new encoding configurations.

x/common/testutil/cli/network.go (1)

160-160: Correctly replaced Marshaler with Codec in the BuildNetworkConfig function to align with the updated encoding configuration.

app/keepers.go (2)

108-108: Ensure the eth import is necessary and used correctly.

The eth package is imported but not used in the provided code. Verify if this import is required or if it can be removed to avoid unnecessary dependencies.


401-402: Verify the initialization of EvmKeeper with AccountKeeper and BankKeeper.

Ensure that AccountKeeper and BankKeeper are correctly initialized and passed to EvmKeeper. This change should be consistent with the overall architecture and design of the application.

Comment on lines +61 to +62
- [#1869](https://github.com/NibiruChain/nibiru/pull/1869) - feat(eth): Module and start of keeper tests
- [#1873](https://github.com/NibiruChain/nibiru/pull/1873) - feat(evm): keeper collections and grpc query impls for EthAccount, NibiruAccount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove trailing space.

- - [#1869](https://github.com/NibiruChain/nibiru/pull/1869) - feat(eth): Module and start of keeper tests 
+ - [#1869](https://github.com/NibiruChain/nibiru/pull/1869) - feat(eth): Module and start of keeper tests

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
- [#1869](https://github.com/NibiruChain/nibiru/pull/1869) - feat(eth): Module and start of keeper tests
- [#1873](https://github.com/NibiruChain/nibiru/pull/1873) - feat(evm): keeper collections and grpc query impls for EthAccount, NibiruAccount
- [#1869](https://github.com/NibiruChain/nibiru/pull/1869) - feat(eth): Module and start of keeper tests
- [#1873](https://github.com/NibiruChain/nibiru/pull/1873) - feat(evm): keeper collections and grpc query impls for EthAccount, NibiruAccount

Comment on lines +284 to +289
// TODO: chore(upgrade): Potential breaking change on AccountKeeper dur
// to ProtoBaseAccount replacement.
app.AccountKeeper = authkeeper.NewAccountKeeper(
appCodec,
keys[authtypes.StoreKey],
authtypes.ProtoBaseAccount,
eth.ProtoBaseAccount,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarify the TODO comment and ensure the initialization of AccountKeeper with eth.ProtoBaseAccount is correct.

The TODO comment should be more specific about the potential breaking change and what needs to be done to address it. Additionally, verify that eth.ProtoBaseAccount is the correct type to use here.

- // TODO: chore(upgrade): Potential breaking change on AccountKeeper dur
- // to ProtoBaseAccount replacement.
+ // TODO: chore(upgrade): Investigate potential breaking change on AccountKeeper due to ProtoBaseAccount replacement and ensure compatibility.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
// TODO: chore(upgrade): Potential breaking change on AccountKeeper dur
// to ProtoBaseAccount replacement.
app.AccountKeeper = authkeeper.NewAccountKeeper(
appCodec,
keys[authtypes.StoreKey],
authtypes.ProtoBaseAccount,
eth.ProtoBaseAccount,
// TODO: chore(upgrade): Investigate potential breaking change on AccountKeeper due to ProtoBaseAccount replacement and ensure compatibility.
app.AccountKeeper = authkeeper.NewAccountKeeper(
appCodec,
keys[authtypes.StoreKey],
eth.ProtoBaseAccount,

@@ -58,7 +58,8 @@
- [#1855](https://github.com/NibiruChain/nibiru/pull/1855) - feat(eth-pubsub): Implement in-memory EventBus for real-time topic management and event distribution
- [#1856](https://github.com/NibiruChain/nibiru/pull/1856) - feat(eth-rpc): Conversion types and functions between Ethereum txs and blocks and Tendermint ones.
- [#1861](https://github.com/NibiruChain/nibiru/pull/1861) - feat(eth-rpc): RPC backend, Ethereum tracer, KV indexer, and RPC APIs
- [#1869](https://github.com/NibiruChain/nibiru/pull/1869) - feat(eth): Module and start of keeper tests
- [#1869](https://github.com/NibiruChain/nibiru/pull/1869) - feat(eth): Module and start of keeper tests
- [#1873](https://github.com/NibiruChain/nibiru/pull/1873) - feat(evm): keeper collections and grpc query impls for EthAccount, NibiruAccount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a period at the end of the entry for consistency.

- - [#1873](https://github.com/NibiruChain/nibiru/pull/1873) - feat(evm): keeper collections and grpc query impls for EthAccount, NibiruAccount
+ - [#1873](https://github.com/NibiruChain/nibiru/pull/1873) - feat(evm): keeper collections and grpc query impls for EthAccount, NibiruAccount.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
- [#1873](https://github.com/NibiruChain/nibiru/pull/1873) - feat(evm): keeper collections and grpc query impls for EthAccount, NibiruAccount
- [#1873](https://github.com/NibiruChain/nibiru/pull/1873) - feat(evm): keeper collections and grpc query impls for EthAccount, NibiruAccount.

Comment on lines +450 to +451

var EmptyCodeHash = crypto.Keccak256(nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define EmptyCodeHash as a constant.

The EmptyCodeHash variable should be defined as a constant since its value is immutable and represents a fixed value.

- var EmptyCodeHash = crypto.Keccak256(nil)
+ const EmptyCodeHash = crypto.Keccak256(nil)

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
var EmptyCodeHash = crypto.Keccak256(nil)
const EmptyCodeHash = crypto.Keccak256(nil)

Comment on lines +13 to +14
// StoreKey: Persistent storage key for ethereum storage data, account code
// (StateDB) or block related data for the Eth Web3 API.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improve comment clarity.

Consider rephrasing the comment for better clarity:

- // StoreKey: Persistent storage key for ethereum storage data, account code
- // (StateDB) or block related data for the Eth Web3 API.
+ // StoreKey: Persistent storage key for Ethereum storage data, account code (StateDB), or block-related data for the Eth Web3 API.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
// StoreKey: Persistent storage key for ethereum storage data, account code
// (StateDB) or block related data for the Eth Web3 API.
// StoreKey: Persistent storage key for Ethereum storage data, account code (StateDB), or block-related data for the Eth Web3 API.

Comment on lines +5 to +6
"bytes"
"fmt"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unnecessary blank line.

Remove the unnecessary blank line between the import statements.

- "bytes"
- 
+ "bytes"

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
"bytes"
"fmt"
"bytes"
"fmt"

Comment on lines +24 to +65
genState evm.GenesisState,
) []abci.ValidatorUpdate {
k.BeginBlock(ctx, abci.RequestBeginBlock{})
k.SetParams(ctx, genState.Params)

if addr := accountKeeper.GetModuleAddress(evm.ModuleName); addr == nil {
panic("the EVM module account has not been set")
}

for _, account := range genState.Accounts {
address := gethcommon.HexToAddress(account.Address)
accAddress := sdk.AccAddress(address.Bytes())
// check that the EVM balance the matches the account balance
acc := accountKeeper.GetAccount(ctx, accAddress)
if acc == nil {
panic(fmt.Errorf("account not found for address %s", account.Address))
}

ethAcct, ok := acc.(eth.EthAccountI)
if !ok {
panic(
fmt.Errorf("account %s must be an EthAccount interface, got %T",
account.Address, acc,
),
)
}
code := gethcommon.Hex2Bytes(account.Code)
codeHash := crypto.Keccak256Hash(code)

// we ignore the empty Code hash checking, see ethermint PR#1234
if len(account.Code) != 0 && !bytes.Equal(ethAcct.GetCodeHash().Bytes(), codeHash.Bytes()) {
s := "the evm state code doesn't match with the codehash\n"
panic(fmt.Sprintf("%s account: %s , evm state codehash: %v, ethAccount codehash: %v, evm state code: %s\n",
s, account.Address, codeHash, ethAcct.GetCodeHash(), account.Code))
}

k.SetCode(ctx, codeHash.Bytes(), code)

for _, storage := range account.Storage {
k.SetState(ctx, address, gethcommon.HexToHash(storage.Key), gethcommon.HexToHash(storage.Value).Bytes())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improve error handling.

The InitGenesis function should handle errors more gracefully instead of using panic. Consider returning an error instead.

func InitGenesis(
	ctx sdk.Context,
	k *keeper.Keeper,
	accountKeeper evm.AccountKeeper,
	genState evm.GenesisState,
) ([]abci.ValidatorUpdate, error) {
	k.BeginBlock(ctx, abci.RequestBeginBlock{})
	k.SetParams(ctx, genState.Params)

	if addr := accountKeeper.GetModuleAddress(evm.ModuleName); addr == nil {
		return nil, fmt.Errorf("the EVM module account has not been set")
	}

	for _, account := range genState.Accounts {
		address := gethcommon.HexToAddress(account.Address)
		accAddress := sdk.AccAddress(address.Bytes())
		// check that the EVM balance matches the account balance
		acc := accountKeeper.GetAccount(ctx, accAddress)
		if acc == nil {
			return nil, fmt.Errorf("account not found for address %s", account.Address)
		}

		ethAcct, ok := acc.(eth.EthAccountI)
		if !ok {
			return nil, fmt.Errorf("account %s must be an EthAccount interface, got %T", account.Address, acc)
		}
		code := gethcommon.Hex2Bytes(account.Code)
		codeHash := crypto.Keccak256Hash(code)

		// we ignore the empty Code hash checking, see ethermint PR#1234
		if len(account.Code) != 0 && !bytes.Equal(ethAcct.GetCodeHash().Bytes(), codeHash.Bytes()) {
			s := "the evm state code doesn't match with the codehash\n"
			return nil, fmt.Errorf("%s account: %s , evm state codehash: %v, ethAccount codehash: %v, evm state code: %s\n",
				s, account.Address, codeHash, ethAcct.GetCodeHash(), account.Code)
		}

		k.SetCode(ctx, codeHash.Bytes(), code)

		for _, storage := range account.Storage {
			k.SetState(ctx, address, gethcommon.HexToHash(storage.Key), gethcommon.HexToHash(storage.Value).Bytes())
		}
	}

	// TODO: impl InitGenesis
	return []abci.ValidatorUpdate{}, nil
}

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
genState evm.GenesisState,
) []abci.ValidatorUpdate {
k.BeginBlock(ctx, abci.RequestBeginBlock{})
k.SetParams(ctx, genState.Params)
if addr := accountKeeper.GetModuleAddress(evm.ModuleName); addr == nil {
panic("the EVM module account has not been set")
}
for _, account := range genState.Accounts {
address := gethcommon.HexToAddress(account.Address)
accAddress := sdk.AccAddress(address.Bytes())
// check that the EVM balance the matches the account balance
acc := accountKeeper.GetAccount(ctx, accAddress)
if acc == nil {
panic(fmt.Errorf("account not found for address %s", account.Address))
}
ethAcct, ok := acc.(eth.EthAccountI)
if !ok {
panic(
fmt.Errorf("account %s must be an EthAccount interface, got %T",
account.Address, acc,
),
)
}
code := gethcommon.Hex2Bytes(account.Code)
codeHash := crypto.Keccak256Hash(code)
// we ignore the empty Code hash checking, see ethermint PR#1234
if len(account.Code) != 0 && !bytes.Equal(ethAcct.GetCodeHash().Bytes(), codeHash.Bytes()) {
s := "the evm state code doesn't match with the codehash\n"
panic(fmt.Sprintf("%s account: %s , evm state codehash: %v, ethAccount codehash: %v, evm state code: %s\n",
s, account.Address, codeHash, ethAcct.GetCodeHash(), account.Code))
}
k.SetCode(ctx, codeHash.Bytes(), code)
for _, storage := range account.Storage {
k.SetState(ctx, address, gethcommon.HexToHash(storage.Key), gethcommon.HexToHash(storage.Value).Bytes())
}
}
func InitGenesis(
ctx sdk.Context,
k *keeper.Keeper,
accountKeeper evm.AccountKeeper,
genState evm.GenesisState,
) ([]abci.ValidatorUpdate, error) {
k.BeginBlock(ctx, abci.RequestBeginBlock{})
k.SetParams(ctx, genState.Params)
if addr := accountKeeper.GetModuleAddress(evm.ModuleName); addr == nil {
return nil, fmt.Errorf("the EVM module account has not been set")
}
for _, account := range genState.Accounts {
address := gethcommon.HexToAddress(account.Address)
accAddress := sdk.AccAddress(address.Bytes())
// check that the EVM balance matches the account balance
acc := accountKeeper.GetAccount(ctx, accAddress)
if acc == nil {
return nil, fmt.Errorf("account not found for address %s", account.Address)
}
ethAcct, ok := acc.(eth.EthAccountI)
if !ok {
return nil, fmt.Errorf("account %s must be an EthAccount interface, got %T", account.Address, acc)
}
code := gethcommon.Hex2Bytes(account.Code)
codeHash := crypto.Keccak256Hash(code)
// we ignore the empty Code hash checking, see ethermint PR#1234
if len(account.Code) != 0 && !bytes.Equal(ethAcct.GetCodeHash().Bytes(), codeHash.Bytes()) {
s := "the evm state code doesn't match with the codehash\n"
return nil, fmt.Errorf("%s account: %s , evm state codehash: %v, ethAccount codehash: %v, evm state code: %s\n",
s, account.Address, codeHash, ethAcct.GetCodeHash(), account.Code)
}
k.SetCode(ctx, codeHash.Bytes(), code)
for _, storage := range account.Storage {
k.SetState(ctx, address, gethcommon.HexToHash(storage.Key), gethcommon.HexToHash(storage.Value).Bytes())
}
}
// TODO: impl InitGenesis
return []abci.ValidatorUpdate{}, nil
}

Comment on lines +26 to +31
k.BeginBlock(ctx, abci.RequestBeginBlock{})
k.SetParams(ctx, genState.Params)

if addr := accountKeeper.GetModuleAddress(evm.ModuleName); addr == nil {
panic("the EVM module account has not been set")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improve error handling.

The InitGenesis function should handle errors more gracefully instead of using panic. Consider returning an error instead.

- if addr := accountKeeper.GetModuleAddress(evm.ModuleName); addr == nil {
- 	panic("the EVM module account has not been set")
- }
+ if addr := accountKeeper.GetModuleAddress(evm.ModuleName); addr == nil {
+ 	return nil, fmt.Errorf("the EVM module account has not been set")
+ }

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
k.BeginBlock(ctx, abci.RequestBeginBlock{})
k.SetParams(ctx, genState.Params)
if addr := accountKeeper.GetModuleAddress(evm.ModuleName); addr == nil {
panic("the EVM module account has not been set")
}
k.BeginBlock(ctx, abci.RequestBeginBlock{})
k.SetParams(ctx, genState.Params)
if addr := accountKeeper.GetModuleAddress(evm.ModuleName); addr == nil {
return nil, fmt.Errorf("the EVM module account has not been set")
}

Comment on lines +38 to +40
if acc == nil {
panic(fmt.Errorf("account not found for address %s", account.Address))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improve error handling.

The InitGenesis function should handle errors more gracefully instead of using panic. Consider returning an error instead.

- if acc == nil {
- 	panic(fmt.Errorf("account not found for address %s", account.Address))
- }
+ if acc == nil {
+ 	return nil, fmt.Errorf("account not found for address %s", account.Address)
+ }

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if acc == nil {
panic(fmt.Errorf("account not found for address %s", account.Address))
}
if acc == nil {
return nil, fmt.Errorf("account not found for address %s", account.Address)
}

Comment on lines +43 to +49
if !ok {
panic(
fmt.Errorf("account %s must be an EthAccount interface, got %T",
account.Address, acc,
),
)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improve error handling.

The InitGenesis function should handle errors more gracefully instead of using panic. Consider returning an error instead.

- if !ok {
- 	panic(
- 		fmt.Errorf("account %s must be an EthAccount interface, got %T",
- 			account.Address, acc,
- 		),
- 	)
- }
+ if !ok {
+ 	return nil, fmt.Errorf("account %s must be an EthAccount interface, got %T", account.Address, acc)
+ }

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if !ok {
panic(
fmt.Errorf("account %s must be an EthAccount interface, got %T",
account.Address, acc,
),
)
}
if !ok {
return nil, fmt.Errorf("account %s must be an EthAccount interface, got %T", account.Address, acc)
}

@Unique-Divine Unique-Divine added the x: evm Relates to Nibiru EVM or the EVM Module label May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x: evm Relates to Nibiru EVM or the EVM Module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant