Cross-chain signature support for Kernel wallet#24
Conversation
This reverts commit b0fa042.
This reverts commit 0a3e3f0.
WalkthroughThis pull request introduces comprehensive updates to a cross-chain ERC-4337 wallet project, focusing on enhancing intent-based transaction capabilities, improving deployment scripts, and expanding testing infrastructure. The changes span multiple files, including workflow configurations, README documentation, Solidity contracts, deployment scripts, and test suites. Key modifications include adding cross-chain support, refactoring deployment processes, updating compiler configurations, and introducing new testing frameworks for intent-based and cross-chain operations. Changes
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Cross-chain signature support for Kernel wallet
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
![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
🔭 Outside diff range comments (1)
.github/workflows/tests.yml (1)
Line range hint
38-65: Mask sensitive environment variables in logs.The environment variable echo statements could expose sensitive information in the logs.
- name: Set environment variables run: | - # echo env variables - echo "BSC_RPC_URL=${{ vars[format('{0}_BSC_RPC_URL', env.PREFIX)] }}" + # Log non-sensitive variables only + echo "Setting up environment variables..." + echo "RPC URLs: [masked]" # ... (remove other echo statements for sensitive data) # set env variables echo "BSC_RPC_URL=${{ vars[format('{0}_BSC_RPC_URL', env.PREFIX)] }}" >> $GITHUB_ENV
🧹 Nitpick comments (14)
script/deploy/KernelPlugins.s.sol (3)
8-30: Possible grammatical correction for clarity in documentation.
The phrase "Unknownligly, the executor is not verified..." seems unclear. Consider rephrasing to improve readability and understanding.- * Unknownligly, the executor is not verified on Tenderly v-nets + * Occasionally, the executor might not be verified on Tenderly v-nets
32-39: Consider parameterizing the SALT for environment flexibility.
Using a constant SALT is fine for deterministic deployments, but you may want to allow different SALT values per environment for better scoping, especially if you deploy multiple versions of this contract across different networks.
60-62: Fix mislabeled log message for Executor.
Currently, you log "Deployed Validator Address:" for both the validator and the executor. Consider updating the label for the executor’s address to avoid confusion.- console2.log("Deployed Validator Address:", executor); + console2.log("Deployed Executor Address:", executor);script/deploy/DeployKernelLib.sol (1)
65-75: Consider supporting constructor arguments.
Currently,getValidatorInitCode()andgetExecutorInitCode()return only the creation bytecode. If a future update requires constructor params, you may extend these methods to append constructor arguments.src/KernelIntentECDSAValidator.sol (2)
99-112: Provide detailed revert messages for higher debuggability.
If_validateSignaturereverts, the cause might be unclear. Consider returning more informative revert messages or codes to aid debugging.
Line range hint
115-128: Potential optimization: mark validateSignature as external.
You could markvalidateSignature(...)asexternalif it’s only meant to be called externally; this sometimes reduces gas costs.test/KernelXChainTest.sol (2)
183-199: Validate destination chain contexts.
Even if you’re working in a single-chain environment, consider verifying that cross-chain parameters (like chain IDs or bridging logic) are tested or mocked to detect potential edge cases in a real multi-chain setting.
240-266: Consider adding test coverage for revert scenarios.
While the code tests successful execution, additional coverage for failure scenarios (e.g., signature mismatch, insufficient funds, partial batch) would improve confidence.test/KernelIntentPlugins.t.sol (1)
466-484: Consider using @param tags for better documentation.While the parameter documentation is informative, using standard @param tags would improve IDE integration and documentation generation.
- /// IKernel.setExecution Changes the execution details for a specific function selector - /// @dev This function can only be called from the EntryPoint contract, the contract owner, or itself - bytes4 selector = IKernel.setExecution.selector; + /// @notice Changes the execution details for a specific function selector + /// @dev This function can only be called from the EntryPoint contract, the contract owner, or itself + /// @param _selector The selector of the function for which execution details are being set + /// @param _executor The executor to be associated with the function selector + /// @param _validator The validator contract responsible for validating operations + /// @param _validUntil The timestamp until which the execution details are valid + /// @param _validAfter The timestamp after which the execution details are valid + bytes4 selector = IKernel.setExecution.selector;foundry.toml (1)
24-25: Consider adjusting optimizer runs based on deployment frequency.The current setting of 200 runs is a good default, but consider:
- Increase for less frequently deployed contracts
- Decrease for contracts that will be deployed many times
🧰 Tools
🪛 GitHub Actions: Solidity Tests
[warning] Using nightly build of Foundry. It is recommended to use the latest stable version.
.github/workflows/tests.yml (1)
Line range hint
115-129: Enhance error message in PR comment.The PR comment could provide more detailed information about the test failures.
- gh pr comment "$PR_URL" --body "Tests failed on pull request from branch $SOURCE_BRANCH to $TARGET_BRANCH. Please fix the issues before merging." + COMMENT="Tests failed on pull request from branch $SOURCE_BRANCH to $TARGET_BRANCH. + +Error details: +- Branch: $SOURCE_BRANCH +- Target: $TARGET_BRANCH +- Run: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + +Please fix the issues before merging." + gh pr comment "$PR_URL" --body "$COMMENT"README.md (3)
20-37: Consider adding technical diagrams.While the textual description is comprehensive, adding technical diagrams would help visualize:
- The interaction flow between different components
- The cross-chain orchestration process
- The relationship between IntentSimpleAccount and Kernel wallet architectures
Would you like me to help create technical diagrams using Mermaid or PlantUML syntax?
95-95: Remove duplicate heading.The word "Deployment" appears twice in succession.
-## Deployment Deployment scripts support both IntentSimpleAccount and Kernel configurations:🧰 Tools
🪛 LanguageTool
[duplication] ~95-~95: Possible typo: you repeated a word.
Context: ...wing command:bash make test## Deployment Deployment scripts support both IntentSimpleAccoun...(ENGLISH_WORD_REPEAT_RULE)
99-101: Improve environment variable format consistency.The environment variable format is inconsistent and could be clearer.
Consider this format:
-<NETWORK>_RPC_URL=<your_rpc_url> -<TEST_PRIVATE_KEY>=<your_private_key> -KERNEL_VERSION=v2.4 # For Kernel deployment +NETWORK_RPC_URL=<your-network-rpc-url> # e.g., GOERLI_RPC_URL +PRIVATE_KEY=<your-wallet-private-key> # Without 0x prefix +KERNEL_VERSION=v2.4 # For Kernel deployment
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
script/deploy/dep-log-intent-factory-v-polygon-8888.logis excluded by!**/*.logscript/deploy/dep-log-intent-simple-account-v-polygon-mainnet-8888.logis excluded by!**/*.logscript/deploy/intent-simple-account-v-mainnet-888-deployment.logis excluded by!**/*.logscript/deploy/kernel-plugins-v-mainnet-888-deployment.logis excluded by!**/*.logscript/deploy/logs/dep-log-intent-kernel-plugins-v-bsc-890.logis excluded by!**/*.logscript/deploy/logs/dep-log-intent-kernel-plugins-v-ethereum-888-.logis excluded by!**/*.logscript/deploy/logs/dep-log-intent-kernel-plugins-v-polygon-8889.logis excluded by!**/*.logscript/deploy/logs/dep-log-intent-simple-account-binance-mainnet-1.logis excluded by!**/*.logscript/deploy/logs/dep-log-intent-simple-account-binance-mainnet-56.logis excluded by!**/*.logscript/deploy/logs/dep-log-intent-simple-account-polygon-mainnet-137.logis excluded by!**/*.logscript/deploy/logs/dep-log-intent-simple-account-v-binance-mainnet-890.logis excluded by!**/*.logscript/deploy/logs/dep-log-intent-simple-account-v-ethereum-mainnet-888.logis excluded by!**/*.logscript/deploy/logs/dep-log-intent-simple-account-v-polygon-mainnet-8889.logis excluded by!**/*.log
📒 Files selected for processing (13)
.github/workflows/tests.yml(6 hunks)README.md(1 hunks)foundry.toml(1 hunks)script/deploy/DeployKernelLib.sol(1 hunks)script/deploy/KernelAccount.s.sol(1 hunks)script/deploy/KernelPlugins.s.sol(1 hunks)script/patch-kernel-submod.sh(1 hunks)src/IntentSimpleAccount.sol(1 hunks)src/KernelIntentECDSAValidator.sol(4 hunks)test/KernelIntentPlugins.t.sol(2 hunks)test/KernelXChainTest.sol(1 hunks)test/SimpleAccount_Poly.t.sol(2 hunks)test/SimpleAccount_XChain.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/SimpleAccount_Poly.t.sol
- test/SimpleAccount_XChain.t.sol
🧰 Additional context used
🪛 GitHub Actions: Solidity Tests
foundry.toml
[warning] Using nightly build of Foundry. It is recommended to use the latest stable version.
test/KernelXChainTest.sol
[error] 1-1: Environment variable 'WALLET_OWNER_KEY' not found in setUp()
🪛 LanguageTool
README.md
[duplication] ~95-~95: Possible typo: you repeated a word.
Context: ...wing command: bash make test ## Deployment Deployment scripts support both IntentSimpleAccoun...
(ENGLISH_WORD_REPEAT_RULE)
🔇 Additional comments (20)
script/deploy/KernelPlugins.s.sol (2)
42-43: Verify correctness of broadcast assumptions.
No private key is explicitly specified, relying on the Foundry environment’s default or environment variables. Verify that this usage aligns with your security policies for production deployments.
48-53: Good approach for ensuring deterministic addresses.
Predicting and logging the computed CREATE2 addresses helps confirm the deployment outcome matches expectations.script/deploy/DeployKernelLib.sol (3)
15-16: Confirm CREATE2 factory address presence on target networks.
Ensure that the deterministic-deployment-proxy at this address is available for every network you plan to deploy to.
28-47: Deployment logic looks sound.
The checks for factory code presence and deterministic address computation provide reliability. Make sure to handle potential collisions if you reuse the same salt.
49-59: Correct implementation of CREATE2 address derivation.
This logic matches the standard CREATE2 formula and ensures consistent address derivation.src/KernelIntentECDSAValidator.sol (4)
16-23: Good approach enabling cross-chain logic via XChainLib.
This import effectively centralizes cross-chain utilities, simplifying maintenance and ensuring a clear separation of concerns.
62-95: Thorough cross-chain operation hashing.
The stepwise approach—parsing cross-chain data, computing the vanilla callData hash, and conditionally combining chain data—seems logically consistent for cross-chain ops.
157-159: Approach for validCaller seems concise.
This simple ownership check for the caller is consistent with your storage pattern.
Line range hint
164-201: Ensure thorough test coverage for critical hashing.
Operations likehashIntentOp,packIntentOp,getSender, andcalldataKeccakcan be tricky. Verify your test suite includes thorough coverage for edge and cross-chain scenarios.✅ Verification successful
Test coverage for critical hashing operations is comprehensive
The codebase includes extensive test coverage forhashIntentOp,packIntentOp,getSender, andcalldataKeccakoperations across multiple test files. Tests cover cross-chain scenarios, edge cases, and different validation modes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for test references to the hashing functions in your test directory fd -e sol --search-path test | xargs rg "hashIntentOp|packIntentOp|getSender|calldataKeccak" -A 5Length of output: 1396
Script:
#!/bin/bash # Search for test functions and assertions related to the hashing operations ast-grep --pattern 'function test$_($$$) $_ { $$$ }' test/ | grep -B2 -A10 "hashIntentOp\|packIntentOp\|getSender\|calldataKeccak" # Also search for require/assert statements near these functions rg "assert|require|revert" -B2 -A2 test/ | grep -B2 -A2 "hashIntentOp\|packIntentOp\|getSender\|calldataKeccak"Length of output: 248
Script:
#!/bin/bash # Find all test files and their contents echo "=== Test Files ===" fd -e sol --search-path test/ echo -e "\n=== Function Usage in Tests ===" fd -e sol --search-path test/ | xargs cat echo -e "\n=== Fuzz Tests ===" rg "function fuzz" -A 5 test/Length of output: 81513
test/KernelXChainTest.sol (2)
145-174: Great structure for single-chain cross-chain simulation tests.
The sequence of steps (enable plugin → prepare userOps → execute them sequentially) is logically sound. This approach enables thorough testing of both source and destination operations in a single environment. There are no obvious logical or security flaws here.
319-359: Careful ordering in cross-chain hashing.
Your approach for ordering and hashing withshouldReverseis clever. However, ensure that any future expansions (like multi-chain lists beyond two) are still accounted for or validated.script/deploy/KernelAccount.s.sol (2)
25-34: Confirm environment variables in the deployment script.
Similar to the tests, you’re usingTEST_PRIVATE_KEYandWALLET_OWNER_KEY. Ensure that your continuous integration environment and local environment set these variables consistently to avoid failures. Consider adding validation steps for missing values.
37-51: Deployment flow looks good.
The script logically retrieves all references, initializes them, and deploys the kernel account. The usage of known addresses is fine, but ensure these addresses reflect any changes in your production environment or new chain deployments.src/IntentSimpleAccount.sol (1)
50-59: Clarity on cross-chain vs. conventional ops.
The updated comments clarify the approach to cross-chain data parsing and the fallback to originalcallDatafor vanilla operations. This maintains readability and correctness.test/KernelIntentPlugins.t.sol (1)
152-161: Well-documented test objectives!The documentation block clearly outlines the test's purpose and verification points, making it easier for other developers to understand the test coverage.
foundry.toml (2)
16-16: Verify compatibility with Solidity 0.8.28.The compiler upgrade from 0.8.27 to 0.8.28 introduces changes. Please ensure all contracts are compatible with the new version.
✅ Verification successful
Solidity 0.8.28 compatibility verified - no issues found
The upgrade from 0.8.27 to 0.8.28 is safe as it contains no breaking changes, and the codebase doesn't use any deprecated or renamed features in contract code. The single
tx.originusage found is in a deployment script for logging purposes only.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any breaking changes between Solidity 0.8.27 and 0.8.28 echo "Checking for breaking changes in contracts..." rg -l "pragma solidity" | while read -r file; do echo "Analyzing $file..." # Look for potential incompatibilities rg -A 5 -B 5 "selfdestruct|block\.difficulty|tx\.origin" "$file" || true doneLength of output: 5554
🧰 Tools
🪛 GitHub Actions: Solidity Tests
[warning] Using nightly build of Foundry. It is recommended to use the latest stable version.
16-25: Address Foundry nightly build warning.The pipeline warning indicates use of a nightly build. Consider switching to the latest stable version for better reliability.
🧰 Tools
🪛 GitHub Actions: Solidity Tests
[warning] Using nightly build of Foundry. It is recommended to use the latest stable version.
README.md (3)
1-4: LGTM! Clear title and useful status badges.The title accurately reflects both wallet implementations, and the badges provide valuable status information for the project.
6-6: Well-structured introduction with clear value proposition.The introduction effectively communicates the project's scope, highlighting both wallet implementations and their key features.
72-73:⚠️ Potential issueFix script path inconsistency.
The script path in the command (
scripts/patch-kernel-submod.sh) doesn't match the directory name mentioned in the text (script/patch-kernel-submod.sh).Please update either the command or ensure the script is in the correct directory:
-chmod +x scripts/patch-kernel-submod.sh -./scripts/patch-kernel-submod.sh +chmod +x script/patch-kernel-submod.sh +./script/patch-kernel-submod.shLikely invalid or redundant comment.
| string memory privateKeyString = vm.envString("WALLET_OWNER_KEY"); | ||
| ownerPrivateKey = vm.parseUint(privateKeyString); | ||
| ownerAddress = vm.addr(ownerPrivateKey); | ||
|
|
||
| console2.log("Owner address:", ownerAddress); | ||
| assertFalse(ownerAddress == address(0), "Owner address should not be the zero address"); | ||
|
|
||
| // Create the polygon fork | ||
| string memory urlEnv = "POLYGON_RPC_URL"; | ||
| polyFork = vm.createFork(vm.envString(urlEnv)); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Update test to use documented environment variable convention
The test is using WALLET_OWNER_KEY but this doesn't match the project's documented environment variable convention. According to both README.md and .env.example, private keys should follow the format <NETWORK>_PRIVATE_KEY (e.g., ETHEREUM_PRIVATE_KEY, POLYGON_PRIVATE_KEY). Update the test to use the correct variable name based on the target network (Polygon in this case).
- In
test/KernelXChainTest.sol, replaceWALLET_OWNER_KEYwithPOLYGON_PRIVATE_KEYto match the network-specific naming convention used throughout the project.
🔗 Analysis chain
Ensure environment variables are correctly configured.
The pipeline fails due to a missing WALLET_OWNER_KEY. Since you're referencing this variable at lines 79-80, you must ensure it's defined in the CI environment or handle the case when it's absent (e.g., provide a fallback or throw a descriptive error message).
Below is a sample script to confirm the variable seems set:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if WALLET_OWNER_KEY is set in the environment
if [ -z "$WALLET_OWNER_KEY" ]; then
echo "WALLET_OWNER_KEY is not set"
exit 1
else
echo "WALLET_OWNER_KEY is set"
fi
Length of output: 89
Script:
#!/bin/bash
# Look for .env files and environment documentation
echo "Searching for .env files and documentation..."
fd -H "\.env" -E "node_modules"
fd "env" -t f -E "node_modules"
echo -e "\nChecking for CI configuration..."
fd "\.github|\.gitlab-ci\.yml|\.circleci" -t f -E "node_modules"
echo -e "\nChecking README for environment setup instructions..."
if [ -f "README.md" ]; then
echo "Content of README.md:"
cat README.md
fi
Length of output: 6776
Script:
#!/bin/bash
# Check content of .env.example
echo "Content of .env.example:"
cat .env.example
Length of output: 423
| #!/bin/bash | ||
|
|
||
| # Initialize all submodules first | ||
| git submodule update --init | ||
|
|
||
| # Fix the ExcessivelySafeCall submodule in kernel | ||
| cd lib/kernel | ||
| git submodule deinit -f -- lib/ExcessivelySafeCall | ||
| rm -rf .git/modules/lib/ExcessivelySafeCall | ||
| git rm -f lib/ExcessivelySafeCall | ||
| git submodule add https://github.com/nomad-xyz/ExcessivelySafeCall lib/ExcessivelySafeCall | ||
| git submodule update --init --recursive | ||
| cd ../.. No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and directory validation.
The script should handle potential failures in Git operations and directory navigation.
#!/bin/bash
+set -euo pipefail
# Initialize all submodules first
-git submodule update --init
+git submodule update --init || { echo "Failed to initialize submodules"; exit 1; }
# Fix the ExcessivelySafeCall submodule in kernel
-cd lib/kernel
+cd lib/kernel || { echo "Failed to change directory to lib/kernel"; exit 1; }
git submodule deinit -f -- lib/ExcessivelySafeCall
rm -rf .git/modules/lib/ExcessivelySafeCall
git rm -f lib/ExcessivelySafeCall
git submodule add https://github.com/nomad-xyz/ExcessivelySafeCall lib/ExcessivelySafeCall
git submodule update --init --recursive
-cd ../..
+cd ../.. || { echo "Failed to return to root directory"; exit 1; }📝 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.
| #!/bin/bash | |
| # Initialize all submodules first | |
| git submodule update --init | |
| # Fix the ExcessivelySafeCall submodule in kernel | |
| cd lib/kernel | |
| git submodule deinit -f -- lib/ExcessivelySafeCall | |
| rm -rf .git/modules/lib/ExcessivelySafeCall | |
| git rm -f lib/ExcessivelySafeCall | |
| git submodule add https://github.com/nomad-xyz/ExcessivelySafeCall lib/ExcessivelySafeCall | |
| git submodule update --init --recursive | |
| cd ../.. | |
| #!/bin/bash | |
| set -euo pipefail | |
| # Initialize all submodules first | |
| git submodule update --init || { echo "Failed to initialize submodules"; exit 1; } | |
| # Fix the ExcessivelySafeCall submodule in kernel | |
| cd lib/kernel || { echo "Failed to change directory to lib/kernel"; exit 1; } | |
| git submodule deinit -f -- lib/ExcessivelySafeCall | |
| rm -rf .git/modules/lib/ExcessivelySafeCall | |
| git rm -f lib/ExcessivelySafeCall | |
| git submodule add https://github.com/nomad-xyz/ExcessivelySafeCall lib/ExcessivelySafeCall | |
| git submodule update --init --recursive | |
| cd ../.. || { echo "Failed to return to root directory"; exit 1; } |
| - name: Initialize and Patch Submodules | ||
| run: | | ||
| cd lib/kernel | ||
| git submodule deinit -f -- lib/ExcessivelySafeCall | ||
| rm -rf .git/modules/lib/ExcessivelySafeCall | ||
| git rm -f lib/ExcessivelySafeCall | ||
| git submodule add https://github.com/nomad-xyz/ExcessivelySafeCall lib/ExcessivelySafeCall | ||
| git submodule update --init --recursive | ||
| cd ../.. | ||
| chmod +x script/patch-kernel-submod.sh | ||
| ./script/patch-kernel-submod.sh |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify script permissions are reset after execution.
The script permissions are changed to executable but not reset afterward, which could be a security concern.
- name: Initialize and Patch Submodules
run: |
chmod +x script/patch-kernel-submod.sh
- ./script/patch-kernel-submod.sh
+ ./script/patch-kernel-submod.sh
+ chmod -x script/patch-kernel-submod.sh📝 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.
| - name: Initialize and Patch Submodules | |
| run: | | |
| cd lib/kernel | |
| git submodule deinit -f -- lib/ExcessivelySafeCall | |
| rm -rf .git/modules/lib/ExcessivelySafeCall | |
| git rm -f lib/ExcessivelySafeCall | |
| git submodule add https://github.com/nomad-xyz/ExcessivelySafeCall lib/ExcessivelySafeCall | |
| git submodule update --init --recursive | |
| cd ../.. | |
| chmod +x script/patch-kernel-submod.sh | |
| ./script/patch-kernel-submod.sh | |
| - name: Initialize and Patch Submodules | |
| run: | | |
| chmod +x script/patch-kernel-submod.sh | |
| ./script/patch-kernel-submod.sh | |
| chmod -x script/patch-kernel-submod.sh |
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores