-
Notifications
You must be signed in to change notification settings - Fork 202
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
Conversation
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
WalkthroughThese changes introduce several enhancements to the Ethereum Virtual Machine (EVM) module within the NibiruChain project. The primary updates include the addition of a Changes
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
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 #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
|
There was a problem hiding this comment.
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
x/evm/query_test.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted some inactive code
There was a problem hiding this 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
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 testseth/state_encoder.go
[warning] 77-77: eth/state_encoder.go#L77
Added line #L77 was not covered by testsx/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 testseth/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 forFunToken
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 usegethcommon
The change from
ethcommon
togethcommon
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 forEthHash
updatedSimilar to the previous comment on
EthAddr
, updating the alias forEthHash
to usegethcommon
is a simple change that maintains consistency with the updated imports.
47-47
: Updated decoding logic for Ethereum addressesThe 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 lengthThe 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 ofFunTokenState
with multi-indexingThe setup of
FunTokenState
usingcollections.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 testseth/hex.go (1)
46-58
: Validation logic inHexAddr.Valid
methodThe
Valid
method effectively ensures that theHexAddr
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
MethodThe
HexAddr.Valid
method is covered by unit tests in theeth/hex_test.go
file. These tests ensure the method's effectiveness across various scenarios.
eth/hex_test.go
: Contains tests for theValid
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_filesLength of output: 42526
x/evm/keeper/keeper.go (1)
Line range hint
56-76
: Enhancements toKeeper
constructor withFunTokenState
initializationThe addition of
FunTokenState
to theKeeper
struct and its initialization in theNewKeeper
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 forFunToken
is well-defined and robust.The use of
gogoproto.customtype
forerc20_addr
enhances type safety and clarity, linking directly to theHexAddr
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 ofHexAddr
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.
@@ -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. |
There was a problem hiding this comment.
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.
// 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) |
There was a problem hiding this comment.
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
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) | ||
}) | ||
} |
There was a problem hiding this comment.
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 likeNewHexAddr
,NewHexAddrFromStr
, orMustNewHexAddr
.
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
// 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 |
There was a problem hiding this comment.
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.
// 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 | ||
} |
There was a problem hiding this comment.
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
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() |
There was a problem hiding this comment.
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.
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() |
There was a problem hiding this 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
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 thego-ethereum/common.Address
(ERC-55 Ethereum address) so that we can use Eth addresses as a gogoproto.customtypeSummary by CodeRabbit
New Features
Bug Fixes
Tests
HexAddr
) and fungible tokens.Documentation
CHANGELOG.md
to include recent additions related to EVM fungible tokens.Chores