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): EVM fungible token protobufs and encoding tests #1936

Merged
merged 9 commits into from
Jun 26, 2024
Merged

Conversation

Unique-Divine
Copy link
Member

@Unique-Divine Unique-Divine commented Jun 24, 2024

Purpose / Abstract

This pull request implements the FunToken type outline in the spec for #1898 along with related key-value stores for reading and writing these coin and ERC20 mappings.

A special string type called HexAddr was added in order to maintain a safe, direct conversion between strings and the go-ethereum/common.Address (ERC-55 Ethereum address) so that we can use Eth addresses as a gogoproto.customtype

Summary by CodeRabbit

  • New Features

    • Introduced support for Ethereum Virtual Machine (EVM) fungible tokens with new protobufs and encoding tests.
    • Added functionality for creating, validating, and managing ERC-20 compatible tokens within the EVM module.
  • Bug Fixes

    • Updated comments for better clarity on contract storage updates and deletions.
  • Tests

    • Added comprehensive test cases for validating Ethereum hexadecimal addresses (HexAddr) and fungible tokens.
  • Documentation

    • Updated CHANGELOG.md to include recent additions related to EVM fungible tokens.
  • Chores

    • Adjusted imports and type aliases to improve code maintainability.

Squashed commit of the following:

commit 51071dad60c12e02093d92d1311d33b70b81304f
Author: Unique-Divine <[email protected]>
Date:   Mon Jun 10 12:32:00 2024 -0500

    refactor(e2e-evm): unused vars and better type hint

commit 6729b97
Author: Unique-Divine <[email protected]>
Date:   Fri Jun 7 22:46:14 2024 -0500

    test(e2e-evm): more type safety

commit 25c1557
Author: Unique-Divine <[email protected]>
Date:   Fri Jun 7 22:01:21 2024 -0500

    Squashed commit of the following:

    commit 021d161d176112cf24e28780ad64f61155f70ea2
    Author: Unique-Divine <[email protected]>
    Date:   Fri Jun 7 22:00:10 2024 -0500

        test(e2e): (1) Generated smart contract types for ethers. (2) TypeScript support. (3) Formatter

    commit af7e7b3
    Author: Unique-Divine <[email protected]>
    Date:   Fri Jun 7 16:11:45 2024 -0500

        chore: another issue ticket

    commit 36745fd
    Author: Unique-Divine <[email protected]>
    Date:   Fri Jun 7 16:07:20 2024 -0500

        chore: add issue number for TODO comment

    commit 8a76c0e
    Author: Unique-Divine <[email protected]>
    Date:   Fri Jun 7 15:54:07 2024 -0500

        refactor(evm): Remove dead code and document non-EVM ante handler

    commit e4e11df
    Author: Unique-Divine <[email protected]>
    Date:   Fri Jun 7 15:52:38 2024 -0500

        refactor: remove dead code

    commit cad00c0
    Merge: dc5f4dd 359e310
    Author: Unique-Divine <[email protected]>
    Date:   Fri Jun 7 15:41:53 2024 -0500

        Merge branch 'main' into ud/ante-test

    commit dc5f4dd
    Author: Unique-Divine <[email protected]>
    Date:   Fri Jun 7 15:28:53 2024 -0500

        refactor: ante handler and evm cleanup

    commit f73cdc3
    Merge: d3a6ea9 290c372
    Author: Unique-Divine <[email protected]>
    Date:   Wed Jun 5 20:59:39 2024 -0500

        Merge branch 'test/evm-grpc-query' of https://github.com/NibiruChain/nibiru into test/evm-grpc-query

    commit d3a6ea9
    Merge: 376596d 70ee1bf
    Author: Unique-Divine <[email protected]>
    Date:   Wed Jun 5 20:59:30 2024 -0500

        Merge branch 'main' into test/evm-grpc-query

    commit 376596d
    Author: Unique-Divine <[email protected]>
    Date:   Wed Jun 5 20:58:40 2024 -0500

        Squashed commit of the following:

        commit b5687130ff5f3d020a3b14d219fec3a816579c30
        Author: Unique-Divine <[email protected]>
        Date:   Wed Jun 5 20:57:44 2024 -0500

            chore: run tidy

        commit 1f1f938
        Merge: 3e3cc83 bbcc6f8
        Author: Unique-Divine <[email protected]>
        Date:   Wed Jun 5 19:16:30 2024 -0500

            Merge branch 'main' into ud/fix-race-condition

        commit 3e3cc83
        Author: Unique-Divine <[email protected]>
        Date:   Wed Jun 5 19:15:40 2024 -0500

            chore: changelog

        commit 3876ccb
        Author: Unique-Divine <[email protected]>
        Date:   Wed Jun 5 19:04:00 2024 -0500

            refactor: more consistent test names

        commit aaa0a19
        Author: Unique-Divine <[email protected]>
        Date:   Wed Jun 5 18:53:09 2024 -0500

            test(oracle): Fix missing tear down step for oracle integration test

        commit 8c3c35e
        Author: Unique-Divine <[email protected]>
        Date:   Wed Jun 5 17:55:56 2024 -0500

            chore: add test comands to justfile

        commit 4916282
        Merge: 64ed0a2 e7e708d
        Author: Unique-Divine <[email protected]>
        Date:   Fri May 31 09:35:33 2024 -0500

            Merge branch 'main' into ud/fix-race-condition

        commit 64ed0a2
        Author: Unique-Divine <[email protected]>
        Date:   Fri May 31 01:44:55 2024 -0500

            fix(gosdk): tests parallel race condition

    commit 290c372
    Merge: 0d1c894 70ee1bf
    Author: Unique Divine <[email protected]>
    Date:   Wed Jun 5 20:05:19 2024 -0500

        Merge branch 'main' into test/evm-grpc-query

    commit 0d1c894
    Merge: 9170835 ad173e9
    Author: Unique Divine <[email protected]>
    Date:   Wed Jun 5 19:34:38 2024 -0500

        Merge branch 'main' into test/evm-grpc-query

    commit 9170835
    Author: Oleg Nikonychev <[email protected]>
    Date:   Wed Jun 5 13:55:14 2024 +0400

        fix: removed hardcoded gas value in grpc_query test

    commit 4337858
    Author: Oleg Nikonychev <[email protected]>
    Date:   Wed Jun 5 13:14:34 2024 +0400

        chore: refactored eth util methods

    commit 7df84e2
    Merge: 8918498 bbcc6f8
    Author: Oleg Nikonychev <[email protected]>
    Date:   Wed Jun 5 12:23:51 2024 +0400

        chore: resolve conflicts

    commit 8918498
    Merge: 3fd45ce e7e708d
    Author: Oleg Nikonychev <[email protected]>
    Date:   Mon Jun 3 21:56:39 2024 +0400

        Merge branch 'main' into test/evm-grpc-query

    commit 3fd45ce
    Author: Oleg Nikonychev <[email protected]>
    Date:   Mon Jun 3 21:56:23 2024 +0400

        chore: changelog update

    commit 3348876
    Author: Oleg Nikonychev <[email protected]>
    Date:   Mon Jun 3 21:53:54 2024 +0400

        test(evm): grpc_query full coverage
Copy link
Contributor

coderabbitai bot commented Jun 24, 2024

Walkthrough

These changes introduce several enhancements to the Ethereum Virtual Machine (EVM) module within the NibiruChain project. The primary updates include the addition of a FunToken message for handling fungible tokens in Protobuf format, corresponding tests for the HexAddr, updates to type handling, initializations in the Keeper struct, and modifications to event attributes. This significantly improves how fungible tokens are managed, validated, and encoded within the system.

Changes

File(s) Summary
CHANGELOG.md Documented the addition of the EVM fungible token protobufs and tests.
e2e/evm/test/contract_send_nibi.test.ts Updated imports and type declarations for SendMethod.
eth/hex.go, eth/hex_test.go Introduced HexAddr type and related functions, along with test cases for Ethereum hex addresses.
eth/state_encoder.go Changed import paths and updated type aliases for Ethereum address and hash handling.
proto/eth/evm/v1/evm.proto Added the FunToken message to represent fungible token mappings.
proto/eth/evm/v1/genesis.proto Introduced funtoken_mappings in GenesisState to hold ERC-20 token mappings.
x/evm/access_list_test.go Added a copyright notice.
x/evm/const.go Added constants for FunToken mappings in the EVM module.
x/evm/events.go Modified several event attribute key constants.
x/evm/evm.go, x/evm/evm_test.go Introduced functions and tests for FunToken handling, including creation and validation.
x/evm/keeper/evm_state.go Clarified the documentation for the SetAccState function.
x/evm/keeper/funtoken_state.go Added FunTokenState struct and methods for managing fungible tokens.
x/evm/keeper/keeper.go Included FunTokens field in Keeper struct and initialized it in NewKeeper function.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant EVM
    participant Keeper
    participant FunTokenState

    User->>EVM: Create FunToken
    EVM->>FunTokenState: Validate and Insert FunToken
    FunTokenState-->>EVM: Validation Result
    EVM-->>User: FunToken Created
Loading

Poem

Whispers in bytes, the tokens flow,
Ethereum's dance, with addresses aglow.
Keeper of keys, with maps anew,
FunTokens flourish, set in their cue.
Nibiru's chain, the ledger's delight,
Under rabbit's watch, systems take flight.


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.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @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 Jun 24, 2024

Codecov Report

Attention: Patch coverage is 60.19417% with 41 lines in your changes missing coverage. Please review.

Project coverage is 64.71%. Comparing base (4a73c37) to head (ae624cb).
Report is 1 commits behind head on main.

Current head ae624cb differs from pull request most recent head 351442a

Please upload reports for the commit 351442a to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1936      +/-   ##
==========================================
- Coverage   64.74%   64.71%   -0.03%     
==========================================
  Files         250      253       +3     
  Lines       15953    16053     +100     
==========================================
+ Hits        10329    10389      +60     
- Misses       4871     4907      +36     
- Partials      753      757       +4     
Files Coverage Δ
x/evm/const.go 0.00% <ø> (ø)
x/evm/keeper/evm_state.go 85.36% <ø> (ø)
x/evm/keeper/keeper.go 75.00% <100.00%> (+0.58%) ⬆️
eth/state_encoder.go 80.95% <66.66%> (ø)
x/evm/evm.go 40.00% <40.00%> (ø)
eth/hex.go 74.07% <74.07%> (ø)
x/evm/keeper/funtoken_state.go 44.00% <44.00%> (ø)

@Unique-Divine Unique-Divine added type: enhancement x: evm Relates to Nibiru EVM or the EVM Module labels Jun 25, 2024
@Unique-Divine Unique-Divine marked this pull request as ready for review June 25, 2024 23:32
@Unique-Divine Unique-Divine requested a review from a team as a code owner June 25, 2024 23:32
Copy link
Member Author

Choose a reason for hiding this comment

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

A more concise type description here that reads the field type from the generated code instead of "hard-coding" it

Copy link
Member Author

Choose a reason for hiding this comment

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

Deleted some inactive code

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

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between cc4ddd4 and ae624cb.

Files ignored due to path filters (2)
  • x/evm/evm.pb.go is excluded by !**/*.pb.go
  • x/evm/genesis.pb.go is excluded by !**/*.pb.go
Files selected for processing (15)
  • CHANGELOG.md (1 hunks)
  • e2e/evm/test/contract_send_nibi.test.ts (1 hunks)
  • eth/hex.go (1 hunks)
  • eth/hex_test.go (1 hunks)
  • eth/state_encoder.go (5 hunks)
  • proto/eth/evm/v1/evm.proto (1 hunks)
  • proto/eth/evm/v1/genesis.proto (1 hunks)
  • x/evm/access_list_test.go (1 hunks)
  • x/evm/const.go (1 hunks)
  • x/evm/events.go (1 hunks)
  • x/evm/evm.go (1 hunks)
  • x/evm/evm_test.go (1 hunks)
  • x/evm/keeper/evm_state.go (1 hunks)
  • x/evm/keeper/funtoken_state.go (1 hunks)
  • x/evm/keeper/keeper.go (3 hunks)
Files skipped from review due to trivial changes (1)
  • x/evm/access_list_test.go
Additional context used
GitHub Check: codecov/patch
x/evm/evm.go

[warning] 17-18: x/evm/evm.go#L17-L18
Added lines #L17 - L18 were not covered by tests


[warning] 21-23: x/evm/evm.go#L21-L23
Added lines #L21 - L23 were not covered by tests


[warning] 26-27: x/evm/evm.go#L26-L27
Added lines #L26 - L27 were not covered by tests


[warning] 51-55: x/evm/evm.go#L51-L55
Added lines #L51 - L55 were not covered by tests

eth/state_encoder.go

[warning] 77-77: eth/state_encoder.go#L77
Added line #L77 was not covered by tests

x/evm/keeper/funtoken_state.go

[warning] 42-43: x/evm/keeper/funtoken_state.go#L42-L43
Added lines #L42 - L43 were not covered by tests


[warning] 49-49: x/evm/keeper/funtoken_state.go#L49
Added line #L49 was not covered by tests


[warning] 56-59: x/evm/keeper/funtoken_state.go#L56-L59
Added lines #L56 - L59 were not covered by tests


[warning] 82-85: x/evm/keeper/funtoken_state.go#L82-L85
Added lines #L82 - L85 were not covered by tests


[warning] 87-88: x/evm/keeper/funtoken_state.go#L87-L88
Added lines #L87 - L88 were not covered by tests

eth/hex.go

[warning] 39-39: eth/hex.go#L39
Added line #L39 was not covered by tests


[warning] 66-67: eth/hex.go#L66-L67
Added lines #L66 - L67 were not covered by tests


[warning] 90-95: eth/hex.go#L90-L95
Added lines #L90 - L95 were not covered by tests


[warning] 111-111: eth/hex.go#L111
Added line #L111 was not covered by tests


[warning] 116-116: eth/hex.go#L116
Added line #L116 was not covered by tests

Additional comments not posted (12)
x/evm/events.go (1)

10-16: New event attributes added.

The addition of these attributes enhances the detail available in event logging, which can be beneficial for debugging and tracking transaction flows. Ensure that these new attributes are being populated and used correctly wherever relevant events are emitted.

e2e/evm/test/contract_send_nibi.test.ts (1)

1-1: Ensure concise type descriptions in tests.

As previously suggested by Unique-Divine, ensure that type descriptions in tests are dynamically derived from the generated code to avoid hard-coding and potential discrepancies as the code evolves.

x/evm/const.go (1)

32-37: New constants for FunToken storage and indexing.

The addition of these constants is essential for managing FunToken data within the EVM module's storage system. Ensure that these constants are used consistently and correctly across all relevant parts of the module to avoid data inconsistencies or access issues.

eth/state_encoder.go (4)

16-16: Type aliasing updated to use gethcommon

The change from ethcommon to gethcommon aligns with the updated import paths and ensures consistency across the project. This is a straightforward change and does not introduce any functional differences.


19-19: Type aliasing for EthHash updated

Similar to the previous comment on EthAddr, updating the alias for EthHash to use gethcommon is a simple change that maintains consistency with the updated imports.


47-47: Updated decoding logic for Ethereum addresses

The updated decoding method now directly utilizes gethcommon.BytesToAddress, which is a more direct and potentially less error-prone approach than manually handling the conversion. This change should ensure better reliability and readability.


68-68: Updated decoding method for Ethereum addresses with key length

The inclusion of gethcommon.AddressLength ensures that the decoding process respects the fixed length of Ethereum addresses, which is crucial for avoiding buffer overflows or data integrity issues.

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

28-54: Initialization of FunTokenState with multi-indexing

The setup of FunTokenState using collections.NewIndexedMap and multi-indexes for ERC-20 addresses and bank denominations is well-structured. This should facilitate efficient queries and updates within the state management.

However, ensure that the lambda functions used for indexing (lines 42 and 49) are covered by tests to verify their correct behavior in edge cases.

Tools
GitHub Check: codecov/patch

[warning] 42-43: x/evm/keeper/funtoken_state.go#L42-L43
Added lines #L42 - L43 were not covered by tests


[warning] 49-49: x/evm/keeper/funtoken_state.go#L49
Added line #L49 was not covered by tests

eth/hex.go (1)

46-58: Validation logic in HexAddr.Valid method

The Valid method effectively ensures that the HexAddr instance represents a valid Ethereum address by checking its format and comparing it against the expected format. This is essential for maintaining data consistency and integrity.

However, ensure that this validation logic is covered by unit tests to confirm its effectiveness across various scenarios.

Verification successful

Unit Test Coverage for HexAddr.Valid Method

The HexAddr.Valid method is covered by unit tests in the eth/hex_test.go file. These tests ensure the method's effectiveness across various scenarios.

  • eth/hex_test.go: Contains tests for the Valid method.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify test coverage for the `Valid` method in `HexAddr`.

# Test: Search for unit tests that specifically test the `Valid` method.
rg 'HexAddr.Valid test'

Length of output: 25



Script:

#!/bin/bash
# Description: Broaden the search for unit tests that specifically test the `Valid` method in `HexAddr`.

# Locate all test files
test_files=$(fd --type f --extension go --glob '*_test.go')

# Search for the `Valid` method references in test files
rg --glob '*_test.go' 'Valid' $test_files

Length of output: 42526

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

Line range hint 56-76: Enhancements to Keeper constructor with FunTokenState initialization

The addition of FunTokenState to the Keeper struct and its initialization in the NewKeeper constructor are significant enhancements that enable better management of fungible token states. This change is well-integrated and appears to follow best practices for dependency injection and initialization.

However, ensure that the new fields and their interactions are thoroughly tested, especially in integration tests where multiple components interact.

proto/eth/evm/v1/evm.proto (1)

9-27: Protobuf definition for FunToken is well-defined and robust.

The use of gogoproto.customtype for erc20_addr enhances type safety and clarity, linking directly to the HexAddr type. This is a good practice in system design to ensure data integrity and ease of maintenance.

eth/hex_test.go (1)

20-168: Comprehensive testing of HexAddr functionality.

The tests cover a wide range of scenarios, ensuring that HexAddr handles different input variations correctly and maintains its expected properties across operations. This thorough testing is crucial for maintaining the robustness of the system.

CHANGELOG.md Show resolved Hide resolved
@@ -128,7 +128,7 @@ func (k Keeper) SetParams(ctx sdk.Context, params evm.Params) {
k.EvmState.ModuleParams.Set(ctx, params)
}

// SetState update contract storage, delete if value is empty.
// SetState updates contract storage and deletes if the value is empty.
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify error handling in state deletion.

The function uses _ = state.AccState.Delete(...) to ignore errors when deleting state. While this is common for cleanup scenarios, it's good practice to at least log errors to aid in debugging potential issues.

- _ = state.AccState.Delete(ctx, collections.Join(addr, stateKey))
+ err := state.AccState.Delete(ctx, collections.Join(addr, stateKey))
+ if err != nil {
+     // Log the error or handle it accordingly
+ }
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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// SetState updates contract storage and deletes if the value is empty.
err := state.AccState.Delete(ctx, collections.Join(addr, stateKey))
if err != nil {
// Log the error or handle it accordingly
}

@@ -74,6 +74,6 @@

func (_ keEthHash) Encode(value EthHash) []byte { return value.Bytes() }
func (_ keEthHash) Decode(bz []byte) (int, EthHash) {
return ethcommon.HashLength, ethcommon.BytesToHash(bz)
return gethcommon.HashLength, gethcommon.BytesToHash(bz)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure coverage for new hash decoding logic

The new line added for decoding Ethereum hashes uses gethcommon.BytesToHash, which is consistent with the updated import paths. However, this line is not covered by tests.

Consider adding unit tests to cover this decoding functionality to ensure it handles various edge cases and inputs correctly.

Tools
GitHub Check: codecov/patch

[warning] 77-77: eth/state_encoder.go#L77
Added line #L77 was not covered by tests

Comment on lines +22 to +77
func (s *TestSuite) TestFunToken() {
for testIdx, tc := range []struct {
bankDenom string
erc20Addr eth.HexAddr
wantErr string
}{
{
// sad: Invalid bank denom
bankDenom: "",
erc20Addr: eth.HexAddr(""),
wantErr: "FunTokenError",
},
{
bankDenom: "unibi",
erc20Addr: eth.MustNewHexAddrFromStr("5aaeb6053f3e94c9b9a09f33669435e7ef1beaed"),
wantErr: "",
},
{
bankDenom: "unibi",
erc20Addr: eth.MustNewHexAddrFromStr("5AAEB6053F3E94C9B9A09F33669435E7EF1BEAED"),
wantErr: "",
},
{
// NOTE: notice how this one errors using the same happy path
// input as above because an unsafe constructor was used.
// Naked type overrides should not be used with eth.HexAddr.
// Always use NewHexAddr, NewHexAddrFromStr, or MustNewHexAddr...
bankDenom: "unibi",
erc20Addr: eth.HexAddr("5aaeb6053f3e94c9b9a09f33669435e7ef1beaed"),
wantErr: "not encoded as expected",
},

{
bankDenom: "ibc/AAA/BBB",
erc20Addr: eth.MustNewHexAddrFromStr("0xE1aA1500b962528cBB42F05bD6d8A6032a85602f"),
wantErr: "",
},
{
bankDenom: "tf/contract-addr/subdenom",
erc20Addr: eth.MustNewHexAddrFromStr("0x6B2e60f1030aFa69F584829f1d700b47eE5Fc74a"),
wantErr: "",
},
} {
s.Run(strconv.Itoa(testIdx), func() {
funtoken := evm.FunToken{
Erc20Addr: tc.erc20Addr,
BankDenom: tc.bankDenom,
}
err := funtoken.Validate()
if tc.wantErr != "" {
s.Require().Error(err, "funtoken %s", funtoken)
return
}
s.Require().NoError(err)
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Unsafe constructor usage of eth.HexAddr is not adequately documented or enforced.

The search results indicate that there are instances of eth.HexAddr usage in the codebase without clear documentation or enforcement of safer practices. The comment in the test file highlights the potential issue but does not enforce it throughout the codebase.

  • File: x/evm/evm_test.go
  • Lines: Multiple instances of eth.HexAddr usage without enforcement of safer constructors like NewHexAddr, NewHexAddrFromStr, or MustNewHexAddr.

Recommendation:

  • Document the unsafe usage of eth.HexAddr in the project's documentation.
  • Enforce the use of safer constructors in the codebase to prevent potential issues.
Analysis chain

Comprehensive testing of FunToken validation logic

This test suite effectively covers various scenarios for FunToken validation, including edge cases with empty strings and incorrect encodings. The use of MustNewHexAddrFromStr in tests is appropriate for simplifying test setup, but ensure that error paths are also tested adequately.

However, the comment on line 47 about unsafe constructor usage is crucial and should be highlighted in the documentation or by enforcing safer practices in the code.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that unsafe constructor usage is documented or enforced as safe.

# Test: Search for mentions of unsafe constructor practices in documentation.
rg --type markdown 'unsafe constructor'

Length of output: 41



Script:

#!/bin/bash
# Description: Verify that unsafe constructor usage is enforced as safe in the codebase.

# Search for the usage of the unsafe constructor `eth.HexAddr` in the codebase.
rg 'eth\.HexAddr\('

# Search for comments or enforcement around the usage of `eth.HexAddr`.
rg -A 3 -B 3 'eth\.HexAddr\('

Length of output: 1685

proto/eth/evm/v1/genesis.proto Show resolved Hide resolved
Comment on lines +14 to +16
// FIXME: Explore problems arrising from ERC1155 creating multiple fungible
// tokens that are valid ERC20s with the same address.
// https://github.com/NibiruChain/nibiru/issues/1933
Copy link
Contributor

Choose a reason for hiding this comment

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

Address the FIXME regarding ERC1155 and ERC20 tokens.

This comment highlights a potential issue with fungible tokens created by ERC1155 contracts that can also be valid ERC20s. This could lead to conflicts or unexpected behaviors in token handling.

x/evm/evm.go Show resolved Hide resolved
Comment on lines +78 to +89
// Insert adds an [evm.FunToken] to state with defensive validation. Errors if
// the given inputs would result in a corrupted [evm.FunToken].
func (fun FunTokenState) SafeInsert(
ctx sdk.Context, erc20 gethcommon.Address, bankDenom string, isMadeFromCoin bool,
) error {
funtoken := evm.NewFunToken(erc20, bankDenom, isMadeFromCoin)
if err := funtoken.Validate(); err != nil {
return err
}
fun.Insert(ctx, funtoken.ID(), funtoken)
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Missing test coverage for SafeInsert method

The SafeInsert method in x/evm/keeper/funtoken_state.go does not appear to have any direct test coverage in the identified test files. It is crucial to add tests to ensure that this method handles all specified behaviors and error conditions correctly.

  • File: x/evm/keeper/funtoken_state.go
  • Method: SafeInsert

Please add appropriate test cases to verify the functionality and error handling of the SafeInsert method.

Analysis chain

Safe insertion method for FunToken

The SafeInsert method includes validation checks before insertion, which is a good practice for maintaining data integrity. However, the insertion logic itself (line 87) should be covered by tests to ensure that it correctly handles all specified behaviors, especially error conditions.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify test coverage for `SafeInsert` method.

# Test: Search for test cases specifically targeting the `SafeInsert` method.
rg 'SafeInsert test'

Length of output: 22



Script:

#!/bin/bash
# Step 1: Find the implementation of the `SafeInsert` method.
ast-grep --lang go --pattern 'func (fun FunTokenState) SafeInsert($_, $_, $_, $_) error {
  $$$
}'

# Step 2: Find potential test files that might contain tests for `SafeInsert`.
fd -e go -x grep -l 'SafeInsert' {}

# Step 3: Check if the `SafeInsert` method is being tested in the identified test files.
for file in $(fd -e go -x grep -l 'SafeInsert' {}); do
  echo "Checking file: $file"
  grep -n 'SafeInsert' $file
done

Length of output: 1132



Script:

#!/bin/bash
# Step 1: List all test files in the `x/evm/keeper` directory.
fd -e go _test.go x/evm/keeper

# Step 2: Inspect each test file for any references to `SafeInsert`.
for file in $(fd -e go _test.go x/evm/keeper); do
  echo "Checking file: $file"
  grep -n 'SafeInsert' $file
done

Length of output: 1006

Tools
GitHub Check: codecov/patch

[warning] 82-85: x/evm/keeper/funtoken_state.go#L82-L85
Added lines #L82 - L85 were not covered by tests


[warning] 87-88: x/evm/keeper/funtoken_state.go#L87-L88
Added lines #L87 - L88 were not covered by tests

Comment on lines +23 to +31
func NewHexAddrFromStr(addr string) (HexAddr, error) {
hexAddr := HexAddr(gethcommon.HexToAddress(addr).Hex())
if !gethcommon.IsHexAddress(addr) {
return hexAddr, fmt.Errorf(
"%s: input \"%s\" is not an ERC55-compliant, 20 byte hex address",
HexAddrError, addr,
)
}
return hexAddr, hexAddr.Valid()
Copy link
Contributor

Choose a reason for hiding this comment

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

Robust error handling in NewHexAddrFromStr

The function NewHexAddrFromStr includes comprehensive error checking to ensure that the input string is a valid ERC55-compliant address. This is crucial for avoiding issues related to invalid address formats being used throughout the system.

Consider adding more detailed error messages or logging to assist in debugging if invalid addresses are provided.

- return hexAddr, fmt.Errorf(
-   "%s: input \"%s\" is not an ERC55-compliant, 20 byte hex address",
-   HexAddrError, addr,
- )
+ return hexAddr, fmt.Errorf(
+   "%s: provided input \"%s\" is not a valid ERC55-compliant Ethereum address. Please check the address format and length.",
+   HexAddrError, addr,
+ )
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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func NewHexAddrFromStr(addr string) (HexAddr, error) {
hexAddr := HexAddr(gethcommon.HexToAddress(addr).Hex())
if !gethcommon.IsHexAddress(addr) {
return hexAddr, fmt.Errorf(
"%s: input \"%s\" is not an ERC55-compliant, 20 byte hex address",
HexAddrError, addr,
)
}
return hexAddr, hexAddr.Valid()
func NewHexAddrFromStr(addr string) (HexAddr, error) {
hexAddr := HexAddr(gethcommon.HexToAddress(addr).Hex())
if !gethcommon.IsHexAddress(addr) {
return hexAddr, fmt.Errorf(
"%s: provided input \"%s\" is not a valid ERC55-compliant Ethereum address. Please check the address format and length.",
HexAddrError, addr,
)
}
return hexAddr, hexAddr.Valid()

@k-yang k-yang enabled auto-merge (squash) June 26, 2024 04:30
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: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ae624cb and 351442a.

Files selected for processing (1)
  • CHANGELOG.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md

@k-yang k-yang merged commit 422c676 into main Jun 26, 2024
15 checks passed
@k-yang k-yang deleted the ud/evm branch June 26, 2024 04:37
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
Status: ✅ Completed
Development

Successfully merging this pull request may close these issues.

2 participants