fix: fun token state marshalling #1947
Conversation
WalkthroughThe updates focus on refining the handling of Ethereum addresses within the EVM module by replacing the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1947 +/- ##
==========================================
+ Coverage 64.71% 64.80% +0.08%
==========================================
Files 253 253
Lines 16053 16050 -3
==========================================
+ Hits 10389 10401 +12
+ Misses 4907 4892 -15
Partials 757 757
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- CHANGELOG.md (1 hunks)
- eth/hex.go (3 hunks)
- eth/hex_test.go (1 hunks)
- eth/state_encoder.go (3 hunks)
- eth/state_encoder_test.go (2 hunks)
- x/evm/evm.go (1 hunks)
- x/evm/keeper/evm_state.go (2 hunks)
- x/evm/keeper/funtoken_state.go (3 hunks)
- x/evm/keeper/funtoken_state_test.go (1 hunks)
Additional context used
GitHub Check: codecov/patch
x/evm/evm.go
[warning] 18-18: x/evm/evm.go#L18
Added line #L18 was not covered by tests
[warning] 21-22: x/evm/evm.go#L21-L22
Added lines #L21 - L22 were not covered by testseth/state_encoder.go
[warning] 69-70: eth/state_encoder.go#L69-L70
Added lines #L69 - L70 were not covered by tests
[warning] 73-73: eth/state_encoder.go#L73
Added line #L73 was not covered by testseth/hex.go
[warning] 91-91: eth/hex.go#L91
Added line #L91 was not covered by tests
Additional comments not posted (15)
x/evm/evm.go (2)
25-35: Good refactor on error handling inValidatemethod.Using
funTokenValidationErrorcentralizes error formatting and enhances maintainability.
21-22: Add test coverage forNewFunTokenID.This function is critical for ensuring unique token IDs but lacks test coverage.
Verification successful
Test coverage for
NewFunTokenIDis confirmed.The
NewFunTokenIDfunction is covered by test cases in thex/evm/keeper/funtoken_state_test.gofile.
x/evm/keeper/funtoken_state_test.go: Lines containingNewFunTokenIDScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the test coverage for `NewFunTokenID` function in `x/evm/evm.go`. # Test: Search for test cases covering the `NewFunTokenID` function. Expect: At least one test case. rg --type python $'test.*NewFunTokenID' x/evm/evm_test.goLength of output: 89
Script:
#!/bin/bash # Description: Verify the test coverage for `NewFunTokenID` function in `x/evm/evm.go`. # Test: Search for test cases covering the `NewFunTokenID` function. Expect: At least one test case. rg --type go 'NewFunTokenID' --glob '*_test.go'Length of output: 451
Tools
GitHub Check: codecov/patch
[warning] 21-22: x/evm/evm.go#L21-L22
Added lines #L21 - L22 were not covered by testsx/evm/keeper/funtoken_state.go (2)
19-19: Improvement on type usage inFunTokenState.Using
[]byteandevm.FunTokendirectly in theIndexedMapsimplifies and potentially enhances performance.
69-72: Proper validation before insertion inSafeInsert.Ensuring data integrity by validating
FunTokenbefore insertion is a best practice.x/evm/keeper/funtoken_state_test.go (3)
11-29: Comprehensive testing for insertion and retrieval inTestInsertAndGet.The test effectively verifies the critical functionality of
FunTokenStateby checking insertion and retrieval operations.
34-60: Effective verification of collect functionality inTestCollect.The test ensures that
FunTokeninstances are correctly retrieved by both bank denomination and ERC20 address, confirming the indexing and collection logic.
63-82: Thorough testing of delete functionality inTestDelete.The test ensures that
FunTokeninstances can be successfully removed and are not retrievable post-deletion, confirming the delete logic.eth/state_encoder_test.go (1)
Line range hint
67-80: Effective testing of Ethereum address encoder inTestEncoderEthAddr.The test ensures bijectivity, verifying that encoding, decoding, and stringification are consistent and correct.
eth/state_encoder.go (2)
21-23: Updated encoders for Ethereum address and hash.The changes align with the PR's objectives to use core Geth types, improving type safety and clarity. However, ensure that these encoders are covered by tests.
Also applies to: 26-26
69-73: Implementation ofkeEthHashencoder.The encoder correctly handles
gethcommon.Hash, converting to bytes and back, and providing a string representation. Ensure this encoder is covered by tests.Tools
GitHub Check: codecov/patch
[warning] 69-70: eth/state_encoder.go#L69-L70
Added lines #L69 - L70 were not covered by tests
[warning] 73-73: eth/state_encoder.go#L73
Added line #L73 was not covered by testseth/hex.go (2)
19-19: RefactoredNewHexAddrto usegethcommon.Address.The change aligns with the PR's objective to use core Geth types. This should ensure better type safety and compatibility with the Ethereum library.
61-61: RefactoredToAddrto returngethcommon.Address.This change is consistent with the updated
NewHexAddrfunction, maintaining consistency across the module.x/evm/keeper/evm_state.go (2)
133-133: UpdatedSetAccStateto usegethcommon.Addressandgethcommon.Hash.This change is part of the refactoring to use core Geth types, which should improve the consistency and reliability of the EVM state management.
143-143: UpdatedGetStateto usegethcommon.Addressandgethcommon.Hash.The consistent use of Geth types across these critical functions helps ensure that the Ethereum state is managed accurately within the Cosmos SDK environment.
eth/hex_test.go (1)
274-281: New test functionTestHexAddrToStringadded.This test ensures that the
Stringmethod ofHexAddrreturns the correct string representation of an Ethereum address. It's a good addition to the test suite as it verifies the integrity of address conversions.
| @@ -77,6 +77,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| - [#1917](https://github.com/NibiruChain/nibiru/pull/1917) - test(e2e-evm): TypeScript support. Type generation from compiled contracts. Formatter for TS code. | |||
| - [#1922](https://github.com/NibiruChain/nibiru/pull/1922) - feat(evm): tracer option is read from the config. | |||
| - [#1936](https://github.com/NibiruChain/nibiru/pull/1936) - feat(evm): EVM fungible token protobufs and encoding tests | |||
| - [#1947](https://github.com/NibiruChain/nibiru/pull/1947) - fix(evm): fix FunToken state marshalling | |||
There was a problem hiding this comment.
Enhance the changelog entry for better clarity.
The current changelog entry for PR 1947 is a bit vague. It would be beneficial for users and developers if the entry could provide a bit more context about what exactly was fixed with the FunToken state marshalling and why it was significant.
- [#1947](https://github.com/NibiruChain/nibiru/pull/1947) - fix(evm): Address an issue with `FunToken` state marshalling that previously led to incorrect state transitions under specific conditions.| @@ -88,7 +88,7 @@ | |||
| // Implements the gogo proto custom type interface. | |||
| // Ref: https://github.com/cosmos/gogoproto/blob/v1.5.0/custom_types.md | |||
| func (h *HexAddr) MarshalTo(data []byte) (n int, err error) { | |||
| bz := []byte{} | |||
| bz := []byte(*h) | |||
There was a problem hiding this comment.
Potential issue in MarshalTo method.
The MarshalTo method seems to unnecessarily recreate the HexAddr from a string. This could be simplified and optimized.
- hexAddr, err := NewHexAddrFromStr(string(bz))
- *h = hexAddr
+ // Removed unnecessary conversion
+ err = nilAdditionally, this line was flagged by codecov as not covered by tests.
Would you like me to help add a test case for this method?
Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: codecov/patch
[warning] 91-91: eth/hex.go#L91
Added line #L91 was not covered by tests
| @@ -15,29 +15,24 @@ | |||
| // tokens that are valid ERC20s with the same address. | |||
| // https://github.com/NibiruChain/nibiru/issues/1933 | |||
| func (fun FunToken) ID() []byte { | |||
| return newFunTokenIDFromStr(fun.Erc20Addr.String(), fun.BankDenom) | |||
| return NewFunTokenID(fun.Erc20Addr, fun.BankDenom) | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Add test coverage for the ID method.
The method is crucial for generating unique identifiers but lacks test coverage. Ensure to include test cases that validate the correctness of the ID method in x/evm/evm.go.
- Method:
IDinx/evm/evm.go
Analysis chain
Add test coverage for the ID method.
The method is crucial for generating unique identifiers but lacks test coverage.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the test coverage for `ID` method in `x/evm/evm.go`.
# Test: Search for test cases covering the `ID` method. Expect: At least one test case.
rg --type python $'test.*ID' x/evm/evm_test.go
Length of output: 78
Script:
#!/bin/bash
# Description: Verify the test coverage for `ID` method in `x/evm/evm.go`.
# Test: Search for test cases covering the `ID` method. Expect: At least one test case.
rg --type go 'ID' x/evm/evm_test.go
Length of output: 35
Script:
#!/bin/bash
# Description: Verify the test coverage for `ID` method in `x/evm/evm.go`.
# Test: Search for test cases covering the `ID` method across all Go test files. Expect: At least one test case.
rg --type go 'ID' --glob '*.go'
Length of output: 57772
Tools
GitHub Check: codecov/patch
[warning] 18-18: x/evm/evm.go#L18
Added line #L18 was not covered by tests
| ) | ||
|
|
||
| type ( | ||
| funtokenPrimaryKeyType = []byte |
There was a problem hiding this comment.
I think the primary key type is easier to follow when reading the collections below than using []byte
Purpose / Abstract
FunTokenmarshallingSummary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests