feat: jsonutils precompiles for wrap contract memo construction #148
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package injective: could not load export data: no export data for "github.com/cosmos/relayer/v2/relayer/codecs/injective"" WalkthroughThis pull request introduces a comprehensive JSON utility system for the Ethereum Virtual Machine (EVM) precompiles. The changes include creating a new Changes
Sequence DiagramsequenceDiagram
participant Contract as ERC20 Wrapper
participant JSONUtils as JSON Utilities Precompile
participant IBC as IBC Transfer
Contract->>JSONUtils: merge_json(dst_json, src_json)
JSONUtils-->>Contract: Merged JSON
Contract->>JSONUtils: stringify_json(merged_json)
JSONUtils-->>Contract: Stringified JSON
Contract->>IBC: Transfer with enhanced memo
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
beer-1
force-pushed
the
feat/wrap-memo
branch
from
42edc1b to
7095fc9
Compare
…it for memo construction go mod tidy
beer-1
force-pushed
the
feat/wrap-memo
branch
from
7095fc9 to
07b499d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #148 +/- ##
==========================================
+ Coverage 57.17% 57.20% +0.02%
==========================================
Files 110 112 +2
Lines 10242 10373 +131
==========================================
+ Hits 5856 5934 +78
- Misses 3540 3578 +38
- Partials 846 861 +15
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
x/evm/precompiles/jsonutils/contract.go (3)
22-28: Consider returning an error instead of panicking on ABI loading failures.
Relying onpanichere can be disruptive. For improved resilience in production environments, consider returning an error from the initialization logic and handling it during module setup or contract registration.
49-60: Graceful handling of unexpected panics.
The recover block ensures Cosmos SDK errors are converted into EVM-compatible ones, but be cautious about other unexpected panics. Consider adding logging here to avoid silently dropping important context from the error.
116-119: Remove or replace the debug printing statement.
Usingfmt.Printlnin production code can clutter logs. Consider using a structured logger or removing this statement if no longer needed.- fmt.Println("err", err) + // log.Debug("jsonutils stringify_json error", "error", err)x/evm/precompiles/jsonutils/types.go (2)
7-14: Consider validating or constraining JSON strings.
Although these struct fields are only used for passing parameters, you might consider adding optional validation or checks to ensure the JSON data is well-formed before processing in the precompile.
16-23: Document the rationale behind gas constants and MaxDepth.
Adding comments to explain whyMERGE_GAS,STRINGIFY_JSON_GAS, andMaxDepthare set to their current values can help future contributors maintain consistency or adjust these constants.x/evm/contracts/i_jsonutils/IJSONUtils.sol (1)
10-22: Enhance interface documentation with examples and parameter descriptions.While the interface is well-structured, it would benefit from more detailed documentation:
- Add
@paramand@returndescriptions- Include example JSON inputs and outputs
- Document any size limitations or error conditions
Example documentation format:
/// @notice Recursively merges two JSON strings /// @param dst_json The destination JSON string to merge into /// @param src_json The source JSON string that takes precedence /// @return The merged JSON string /// @dev Example: /// dst_json: {"a": 1, "b": {"c": 2}} /// src_json: {"b": {"d": 3}, "e": 4} /// returns: {"a": 1, "b": {"c": 2, "d": 3}, "e": 4}x/evm/precompiles/jsonutils/merge.go (1)
37-58: Document merge strategy and add array handling.The merge function needs:
- Documentation of the merge strategy for conflicts
- Explicit handling of array values
- Definition of MaxDepth constant
Consider this implementation:
+// MaxDepth defines the maximum recursion depth for JSON merging +const MaxDepth = 32 + +// merge recursively merges src into dst. +// For conflicts: +// - Objects are merged recursively +// - Arrays from src replace arrays in dst +// - Other values from src replace values in dst func merge(dst, src map[string]interface{}, depth int) (map[string]interface{}, error) { if depth > MaxDepth { return nil, errors.New("max recursion depth reached") } for key, srcVal := range src { if dstVal, ok := dst[key]; ok { + // Handle arrays explicitly + if srcArr, srcIsArr := srcVal.([]interface{}); srcIsArr { + dst[key] = srcArr + continue + } srcMap, srcMapOk := mapify(srcVal) dstMap, dstMapOk := mapify(dstVal) if srcMapOk && dstMapOk {x/evm/precompiles/jsonutils/contract_test.go (2)
49-119: Add test cases for edge cases.Consider adding test cases for:
- Empty strings
- Large JSON objects approaching size limits
- JSON arrays
- Non-object JSON values (numbers, strings, null)
- Maximum nesting depth
Example test cases to add:
{ name: "invalid src json", dst: `{"a": 1, "b": 2}`, src: `{"b": 3, "c": 4`, expectedErr: true, }, + { + name: "empty strings", + dst: "", + src: "", + expectedErr: true, + }, + { + name: "non-object json", + dst: `42`, + src: `{"a": 1}`, + expectedErr: true, + }, + { + name: "array in json", + dst: `{"a": [1,2]}`, + src: `{"a": [3,4]}`, + expected: `{"a":[3,4]}`, + },
121-169: Add test cases for special characters and empty structures.Consider adding test cases for:
- Empty objects and arrays
- Special characters (quotes, backslashes)
- Unicode characters
- Null values
Example test cases to add:
{ name: "slice", input: `[1,2,3]`, expected: `"[1,2,3]"`, }, + { + name: "empty structures", + input: `{"a":{},"b":[]}`, + expected: `"{\"a\": {}, \"b\": []}"`, + }, + { + name: "special characters", + input: `{"a":"quote\"here","b":"back\\slash"}`, + expected: `"{\"a\": \"quote\\\"here\", \"b\": \"back\\\\slash\"}"`, + }, + { + name: "unicode", + input: `{"a":"🚀","b":"世界"}`, + expected: `"{\"a\": \"🚀\", \"b\": \"世界\"}"`, + },x/evm/keeper/precompiles_test.go (1)
209-227: Add more comprehensive integration test cases.The integration test should cover:
- Error cases (invalid JSON, insufficient gas)
- Complex JSON structures (nested objects, arrays)
- Edge cases (empty objects, maximum nesting)
Example test cases to add:
func Test_JSONMerge(t *testing.T) { testCases := []struct { + name string + dst string + src string + gasLimit uint64 + expectErr bool + expected string + }{ + { + name: "simple merge", + dst: `{"a": 1, "b": 2}`, + src: `{"b": 3, "c": 4}`, + gasLimit: types.JSONUtilsPrecompileGasCost, + expected: `{"a":1,"b":3,"c":4}`, + }, + { + name: "insufficient gas", + dst: `{"a": 1}`, + src: `{"b": 2}`, + gasLimit: 100, + expectErr: true, + }, + { + name: "complex nested", + dst: `{"a":{"b":{"c":1}}}`, + src: `{"a":{"b":{"d":2}}}`, + gasLimit: types.JSONUtilsPrecompileGasCost, + expected: `{"a":{"b":{"c":1,"d":2}}}`, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { ctx, input := createDefaultTestInput(t) _, _, addr := keyPubAddr() evmAddr := common.BytesToAddress(addr.Bytes())x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (1)
212-224: Consider using a more robust JSON construction approach.While the current implementation works, constructing JSON through string concatenation can be error-prone and harder to maintain. Consider using a structured approach:
- Define a struct for the callback data
- Use a JSON encoding library to convert the struct to JSON
- Then merge with the user memo
This would provide better type safety and maintainability.
Example approach:
+struct AsyncCallback { + uint64 id; + address contract_address; +} + +struct CallbackWrapper { + AsyncCallback async_callback; +} + +struct MemoWrapper { + CallbackWrapper evm; +} function _ibc_transfer(...) internal returns (string memory message) { - string memory callback_memo = string( - abi.encodePacked( - '{"evm": {"async_callback": {"id": ', - Strings.toString(callBackId), - ',"contract_address":"', - Strings.toHexString(address(this)), - '"}}}' - ) - ); + MemoWrapper memory wrapper = MemoWrapper({ + evm: CallbackWrapper({ + async_callback: AsyncCallback({ + id: callBackId, + contract_address: address(this) + }) + }) + }); + string memory callback_memo = JSONUTILS_CONTRACT.encode_json(wrapper);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
x/evm/contracts/erc20_wrapper/ERC20Wrapper.go(2 hunks)x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol(5 hunks)x/evm/contracts/i_jsonutils/IJSONUtils.go(1 hunks)x/evm/contracts/i_jsonutils/IJSONUtils.sol(1 hunks)x/evm/keeper/precompiles.go(2 hunks)x/evm/keeper/precompiles_test.go(2 hunks)x/evm/precompiles/jsonutils/common_test.go(1 hunks)x/evm/precompiles/jsonutils/contract.go(1 hunks)x/evm/precompiles/jsonutils/contract_test.go(1 hunks)x/evm/precompiles/jsonutils/merge.go(1 hunks)x/evm/precompiles/jsonutils/merge_test.go(1 hunks)x/evm/precompiles/jsonutils/types.go(1 hunks)x/evm/types/address.go(1 hunks)
🔇 Additional comments (13)
x/evm/precompiles/jsonutils/contract.go (2)
130-132: Confirm the zero gas cost in RequiredGas.
While gas is consumed inExtendedRun, returning zero here might cause confusion or incorrect assumptions for tools expecting non-zero costs. Double-check if a nominal value or an explanatory comment is preferred.
135-137: Method clarity on Run usage.
The comment clarifies thatRunis not intended for this precompile. The implementation is consistent with the rest of the design.x/evm/contracts/i_jsonutils/IJSONUtils.go (1)
1-3: No manual changes are recommended for auto-generated code.
This file is auto-generated from the Solidity contract. If any changes are needed, consider updating the Solidity interface or the code generator configuration instead.x/evm/contracts/i_jsonutils/IJSONUtils.sol (1)
4-8: LGTM! Well-structured precompile address definition.The constant address definition and contract instance are properly defined at the module level.
x/evm/precompiles/jsonutils/merge_test.go (1)
38-44: LGTM! Well-structured test implementation.The test implementation follows good practices:
- Table-driven tests with clear structure
- Proper error handling and assertions
- Clear test case names
x/evm/types/address.go (1)
25-27: LGTM! Consistent precompile address allocation.The JSONUtils precompile address is properly allocated and follows the established pattern for precompile addresses.
x/evm/keeper/precompiles.go (1)
38-42: LGTM! Clean precompile integration.The JSONUtils precompile is properly initialized and integrated into the precompiles map, following the established pattern.
Also applies to: 47-47
x/evm/precompiles/jsonutils/merge.go (1)
60-72: LGTM!The mapify function safely handles map type conversion using reflection.
x/evm/precompiles/jsonutils/common_test.go (1)
20-252: LGTM!The MockStateDB implementation correctly handles the required functionality for testing (context and snapshots) while appropriately marking other methods as unimplemented.
x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (3)
11-11: LGTM: Import added for JSON utilities.The import statement for IJSONUtils is correctly added to support the new JSON manipulation functionality.
67-67: LGTM: Parameter renamed for better clarity.The parameter rename from
move_memotomemobetter reflects its general purpose usage.
250-252: LGTM: Proper integration of processed memo.The stringified memo is correctly integrated into the final IBC transfer message structure.
x/evm/contracts/erc20_wrapper/ERC20Wrapper.go (1)
478-494: LGTM: Go bindings correctly updated.The auto-generated Go bindings are properly updated to reflect the Solidity contract changes, with correct parameter renaming from
move_memotomemo.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
x/evm/precompiles/jsonutils/types.go (2)
7-10: Consider adding input validation and using more idiomatic field names.
- Field naming could be more Go-idiomatic (e.g.,
DestJSONinstead ofDstJSON).- Consider adding size validation using
MaxJSONSizeconstant.type MergeJSONArguments struct { - DstJSON string `abi:"dst_json"` - SrcJSON string `abi:"src_json"` + DestJSON string `abi:"dst_json"` + SourceJSON string `abi:"src_json"` }
12-14: Consider adding input validation.The JSON input should be validated against
MaxJSONSizeto prevent processing of oversized inputs.x/evm/precompiles/jsonutils/merge_test.go (1)
56-62: Consider adding error test cases.While the happy path is well tested, consider adding test cases for error scenarios:
- Invalid JSON syntax
- Exceeding MaxJSONSize
- Exceeding MaxDepth with deeply nested objects
Example additional test cases:
{ name: "invalid json syntax", dst: `{"a": 1`, src: `{"b": 2}`, err: "unexpected end of JSON input", }, { name: "max depth exceeded", dst: `{"a": {"b": {"c": {"d": {"e": 1}}}}}`, src: `{"a": {"b": {"c": {"d": {"e": 2}}}}}`, err: "max recursion depth reached", }x/evm/precompiles/jsonutils/merge.go (2)
44-65: Consider optimizing the merge function.The current implementation has a few potential improvements:
- Memory efficiency: Consider pre-allocating the result map with capacity hints
- Performance: Avoid unnecessary map conversions for non-map values
func merge(dst, src map[string]interface{}, depth int) (map[string]interface{}, error) { if depth > MaxDepth { return nil, errors.New("max recursion depth reached") } + // Pre-allocate result map with capacity hint + result := make(map[string]interface{}, len(dst)+len(src)) + + // Copy dst to result first + for k, v := range dst { + result[k] = v + } + for key, srcVal := range src { - if dstVal, ok := dst[key]; ok { + if dstVal, ok := result[key]; ok { + // Quick check if either value is not a map + if reflect.TypeOf(srcVal).Kind() != reflect.Map || + reflect.TypeOf(dstVal).Kind() != reflect.Map { + result[key] = srcVal + continue + } srcMap, srcMapOk := mapify(srcVal) dstMap, dstMapOk := mapify(dstVal) if srcMapOk && dstMapOk { var err error if srcVal, err = merge(dstMap, srcMap, depth+1); err != nil { return nil, err } + result[key] = srcVal + continue } } - dst[key] = srcVal + result[key] = srcVal } - return dst, nil + return result, nil }
67-79: Consider adding type assertion optimization.The
mapifyfunction could be optimized by attempting a direct type assertion before using reflection.func mapify(i interface{}) (map[string]interface{}, bool) { + // Try direct type assertion first + if m, ok := i.(map[string]interface{}); ok { + return m, true + } + value := reflect.ValueOf(i) if value.Kind() == reflect.Map { m := map[string]interface{}{} for _, k := range value.MapKeys() { m[k.String()] = value.MapIndex(k).Interface() } return m, true } return map[string]interface{}{}, false }app/upgrade.go (1)
Line range hint
1-119: Consider documenting upgrade requirements.While the upgrade implementation is solid, consider adding documentation comments that outline:
- The new JSON precompile feature being introduced
- Any state migrations or updates required
- Dependencies between the ERC20 wrapper contract updates and JSON precompile
This will help maintainers understand the upgrade's scope and requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/upgrade.go(1 hunks)x/evm/precompiles/jsonutils/merge.go(1 hunks)x/evm/precompiles/jsonutils/merge_test.go(1 hunks)x/evm/precompiles/jsonutils/types.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run test and upload codecov
🔇 Additional comments (9)
x/evm/precompiles/jsonutils/types.go (2)
1-5: LGTM!Package name and import are well-structured.
16-24: 🛠️ Refactor suggestionReview gas costs and constant naming conventions.
Several considerations for the constants:
Gas costs seem low for complex JSON operations:
MERGE_GASandSTRINGIFY_JSON_GASare set to 100, which might not be sufficient for complex JSON structures- Consider factoring in JSON depth in gas calculation
Naming conventions:
- Use Go-style naming for constants (e.g.,
MergeGasinstead ofMERGE_GAS)- Consider more descriptive names (e.g.,
StringifyGasinstead ofSTRINGIFY_JSON_GAS)Security considerations:
- Evaluate if 10KB (
MaxJSONSize) is an appropriate limit for your use case- Consider adding a minimum depth limit alongside
MaxDepthconst ( - MERGE_GAS storetypes.Gas = 100 - STRINGIFY_JSON_GAS storetypes.Gas = 100 + MergeGas storetypes.Gas = 100 + StringifyGas storetypes.Gas = 100 - GAS_PER_BYTE storetypes.Gas = 1 + GasPerByte storetypes.Gas = 1 - MaxDepth = 16 - MaxJSONSize = 10 * 1024 + MinDepth = 1 + MaxDepth = 16 + MaxJSONSize = 10 * 1024 // 10KB )Let's verify the gas costs against similar operations in other precompiles:
x/evm/precompiles/jsonutils/merge_test.go (1)
12-54: LGTM! Comprehensive test coverage with good edge cases.The test cases cover essential scenarios including:
- Simple merges
- Nested object handling
- Conflict resolution
- Empty objects
- Null values
- Array handling
x/evm/precompiles/jsonutils/merge.go (2)
10-42: LGTM! Good input validation and error handling.The implementation includes proper validation for:
- Empty strings
- Size limits
- JSON unmarshaling errors
16-16: Verify MaxJSONSize constant is defined.The code references
MaxJSONSizebut its definition is not visible in the provided files.✅ Verification successful
MaxJSONSize constant is properly defined
The constant is defined in
x/evm/precompiles/jsonutils/types.gowith a value of 10KB (10 * 1024).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for MaxJSONSize constant definition rg -l "MaxJSONSize.*=.*\d+"Length of output: 66
Script:
#!/bin/bash # View the MaxJSONSize definition rg "MaxJSONSize.*=.*\d+" -A 2 x/evm/precompiles/jsonutils/types.goLength of output: 95
Script:
#!/bin/bash # View the complete constant declaration block rg "const.*\(|MaxJSONSize.*=.*\d+" -B 2 -A 2 x/evm/precompiles/jsonutils/types.goLength of output: 225
app/upgrade.go (4)
25-25: LGTM! Version increment follows semantic versioning.The patch version increment from "0.6.9" to "0.6.10" aligns with the introduction of new features (JSON precompile) mentioned in the PR objectives.
Line range hint
71-119: LGTM! Comprehensive contract update process.The update process thoroughly handles all aspects of contract state:
- Contract code replacement
- Code hash updates
- Code size updates
- Account state management
The error handling is robust, and the walker pattern ensures all contracts are processed.
Line range hint
49-67: Verify integration with new JSON precompile.Since this PR introduces JSON precompile features for memo construction, ensure that:
- The ERC20 wrapper contract deployment includes the new JSON precompile address.
- The wrapper contract's bytecode is updated to use the new JSON utilities.
✅ Verification successful
JSON precompile integration verified successfully
The ERC20Wrapper contract correctly integrates with the JSON precompile:
- Imports and uses IJSONUtils interface
- Properly utilizes JSON utilities for memo handling in IBC transfers
- Precompile address (0xf3) is automatically available through the EVM system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the wrapper contract uses the JSON precompile ast-grep --pattern 'contract ERC20Wrapper { $$$ JSONUtils $_ = JSONUtils($_); $$$ }' # Search for JSON precompile address configuration rg -A 2 'JSONUtilsPrecompileAddress|PrecompileAddress' --type goLength of output: 2743
Script:
#!/bin/bash # Find ERC20Wrapper contract fd -e sol "ERC20Wrapper" # Check deployment implementation rg -A 5 "DeployERC20Wrapper" --type go # Look for JSON precompile integration in deployment rg -A 5 "JSONUtilsPrecompileAddress.*wrapper" --type goLength of output: 1487
Script:
#!/bin/bash # View ERC20Wrapper contract content cat x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol # Check deployment implementation details rg -B 5 -A 15 "func \(k Keeper\) DeployERC20Wrapper" --type goLength of output: 9492
Line range hint
38-45: Verify the default gas value for hooks.The code defensively sets
HookMaxGastoDefaultHookMaxGasif it's zero. Please ensure this default value is sufficient for the new JSON precompile operations that will be used in hooks.
Description
Introduce json precompile for the following features
Update wrapper contract to use json precompile at memo construction
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
Release Notes
New Features
merge_jsonandstringify_jsonfunctions for smart contract interactions.Improvements
move_memotomemofor improved clarity.Technical Updates