feat(evm): query fungible token mappings by cosmos denom or ERC20 address #1949
Conversation
WalkthroughThe recent updates introduce a new feature in the Ethereum Virtual Machine (EVM) component, which allows querying fungible token mappings by either coin denomination or ERC-20 contract address. This functionality includes modifications across various files, adding new methods, RPC services, and relevant tests to ensure seamless integration and proper validation of the newly added feature. Changes
Assessment against linked issues
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1949 +/- ##
==========================================
+ Coverage 64.71% 64.83% +0.11%
==========================================
Files 253 253
Lines 16053 16065 +12
==========================================
+ Hits 10389 10416 +27
+ 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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
x/evm/query.pb.gois excluded by!**/*.pb.gox/evm/query.pb.gw.gois excluded by!**/*.pb.gw.go
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- eth/rpc/backend/mocks/evm_query_client.go (1 hunks)
- proto/eth/evm/v1/query.proto (2 hunks)
- x/evm/keeper/grpc_query.go (1 hunks)
- x/evm/keeper/grpc_query_test.go (1 hunks)
Additional context used
Gitleaks
x/evm/keeper/grpc_query_test.go
952-952: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
GitHub Check: lint
x/evm/keeper/grpc_query_test.go
[failure] 918-918:
Error return value ofdeps.K.FunTokens.SafeInsertis not checked (errcheck)
[failure] 943-943:
Error return value ofdeps.K.FunTokens.SafeInsertis not checked (errcheck)
Additional comments not posted (7)
proto/eth/evm/v1/query.proto (2)
79-81: New RPC EndpointTokenMappingAddedThe addition of the
TokenMappingRPC endpoint is consistent with the PR objectives to allow querying fungible token mappings by cosmos denom or ERC20 address. The specified HTTP GET path seems appropriate for the functionality.
309-323: Review ofQueryTokenMappingRequestandQueryTokenMappingResponseMessage DefinitionsThe
QueryTokenMappingRequestandQueryTokenMappingResponsemessage types are well-defined. The request includes a single field for the token (either a cosmos denom or ERC20 address), and the response returns aFunTokenobject. These changes align with the PR's goal to enable querying of token mappings.It's good that the
goproto_gettersoption is set to false, reducing unnecessary accessor methods in Go, which matches the style of other messages in this file.eth/rpc/backend/mocks/evm_query_client.go (1)
380-408: New Mock MethodTokenMappingAddedThe addition of the
TokenMappingmethod to theEVMQueryClientmock is consistent with the introduction of the new RPC method in theQueryservice. This mock method appears to be correctly structured to simulate the behavior of the actual service method, including handling of context and options, and returning the expected types.The method uses the
Calledfunction from the testify/mock package to simulate calling the actual method, which is a standard practice for mocks in Go.x/evm/keeper/grpc_query.go (1)
744-769: New Keeper MethodTokenMappingImplementation ReviewThe
TokenMappingmethod correctly implements the functionality to query fungible token mappings by either cosmos denom or ERC20 address, aligning with the PR objectives. The method first attempts to find a mapping by cosmos denom and then by ERC20 address if the first lookup fails. This dual lookup strategy is efficient and covers the required functionality.However, consider adding a comment explaining the assumption that there is only one mapping for a given denom or ERC20 address, as this could be a significant assumption affecting the method's behavior.
x/evm/keeper/grpc_query_test.go (2)
898-984: Review ofTestQueryTokenMappingfunction.The test scenarios within
TestQueryTokenMappingare well-structured and cover both error and success cases adequately. The tests check for scenarios where a token mapping does not exist and where it does for both a cosmos coin and an ERC20 token. These tests are crucial for validating the correctness of the token mapping feature.Tools
Gitleaks
952-952: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
GitHub Check: lint
[failure] 918-918:
Error return value ofdeps.K.FunTokens.SafeInsertis not checked (errcheck)
[failure] 943-943:
Error return value ofdeps.K.FunTokens.SafeInsertis not checked (errcheck)
952-952: Security Concern: Potential exposure of sensitive data.Line 952 contains a hardcoded ERC20 address. While this might not be an immediate security risk, static analysis has flagged it as a potential Generic API Key. Ensure this is not sensitive or confidential information exposed in the test code.
Tools
Gitleaks
952-952: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
CHANGELOG.md (1)
81-81: Changelog Entry for PR #1949 is Correct and Well-FormattedThe changelog entry for the new feature allowing fungible token mapping queries is concise, adheres to the stated changelog guidelines, and properly references the PR. This ensures good traceability and documentation practices.
x/evm/keeper/grpc_query_test.go
Outdated
| { | ||
| name: "happy: token mapping exists from cosmos coin -> ERC20 token", | ||
| setup: func(deps *evmtest.TestDeps) { | ||
| deps.K.FunTokens.SafeInsert( |
There was a problem hiding this comment.
Handle potential errors from SafeInsert method.
The method deps.K.FunTokens.SafeInsert is called without checking the returned error. It's crucial to handle errors to ensure the robustness of the test setup.
918a919
+ s.Require().NoError(err, "Failed to insert token mapping")
943a944
+ s.Require().NoError(err, "Failed to insert token mapping")Also applies to: 943-943
Tools
GitHub Check: lint
[failure] 918-918:
Error return value ofdeps.K.FunTokens.SafeInsertis not checked (errcheck)
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/evm/keeper/grpc_query_test.go (1 hunks)
Additional context used
Gitleaks
x/evm/keeper/grpc_query_test.go
952-952: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (2)
x/evm/keeper/grpc_query_test.go (2)
898-898: New test function added for querying token mappings.This function is crucial for validating the new feature introduced in this PR, ensuring that the mapping queries are correctly handled both when mappings exist and when they do not.
952-952: Potential security risk: Detected a Generic API Key.The line contains an ERC20 address which is flagged as a Generic API Key by static analysis tools. This might be a false positive, but it's good practice to verify the sensitivity of such data and ensure it's not exposing any sensitive operations or services.
Verification successful
Potential security risk: Detected a Generic API Key.
The ERC20 address "0xAEf9437FF23D48D73271a41a8A094DEc9ac71477" appears to be used exclusively in test files (
x/evm/keeper/funtoken_state_test.goandx/evm/keeper/grpc_query_test.go). These files are typically used for testing purposes and do not usually interact with live systems or sensitive operations.
x/evm/keeper/funtoken_state_test.gox/evm/keeper/grpc_query_test.goSince these are test files, the risk is minimal. However, it's always good to ensure that such addresses are not used in any sensitive contexts outside of testing.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the sensitivity of the ERC20 address flagged as a potential security risk. # Test: Search for the ERC20 address usage across the codebase. Expect: No sensitive usage. rg --type go "0xAEf9437FF23D48D73271a41a8A094DEc9ac71477"Length of output: 1333
Tools
Gitleaks
952-952: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| { | ||
| name: "happy: token mapping exists from cosmos coin -> ERC20 token", | ||
| setup: func(deps *evmtest.TestDeps) { | ||
| _ = deps.K.FunTokens.SafeInsert( |
There was a problem hiding this comment.
Handle potential errors from SafeInsert method.
It is crucial to handle errors to ensure the robustness of the test setup, especially when inserting token mappings which could fail due to database issues or data validation errors.
918a919
+ s.Require().NoError(err, "Failed to insert token mapping")
943a944
+ s.Require().NoError(err, "Failed to insert token mapping")Also applies to: 943-943
Purpose / Abstract
FunTokenmapping by coin denom or ERC20 contract address #1940unibi) or an ERC20 address (e.g.0xAEf9437FF23D48D73271a41a8A094DEc9ac71477)Summary by CodeRabbit