feat(btc-staking): UTXOGateway and BTC staking test #132
Conversation
WalkthroughThis PR brings extensive updates across configuration files, Solidity contracts, deployment scripts, tests, and documentation. Import paths have been updated from Changes
Sequence Diagram(s)sequenceDiagram
participant D as Deployer
participant S as Deploy Script
participant F as CREATE3Factory
participant G as UTXOGateway
D->>S: Invoke deploy-and-setup-utxogateway.script.js
S->>S: Check existing deployment JSON file
S->>F: Validate/Create CREATE3 Factory
S->>G: Deploy UTXOGateway via CREATE3
G-->>S: Return deployed contract address
S->>S: Update JSON file with new addresses
S->>D: Log deployment summary
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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 (
|
adu-web3
force-pushed
the
btc-gateway
branch
from
7e98598 to
698feb0
Compare
|
✅ The Slither Analysis workflow has completed successfully. Check the workflow run for details. (a06ad8a) |
|
✅ The Solhint workflow has completed successfully. Check the workflow run for details. (a06ad8a) |
|
✅ The Forge CI workflow has completed successfully. Check the workflow run for details. (a06ad8a) |
|
✅ The Compare Storage Layouts workflow has completed successfully. Check the workflow run for details. (a06ad8a) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (3)
script/deployBeaconOracle.s.sol (1)
Line range hint
21-24: Consider parameterizing hardcoded values.The beacon oracle address and timestamp are hardcoded:
- The beacon oracle address
0xd3D285cd1516038dAED61B8BF7Ae2daD63662492should be configurable- The timestamp
1_715_918_948(May 17, 2024) is a future date and should be dynamically setConsider:
- Moving the beacon oracle address to a configuration file
- Using a dynamic timestamp based on deployment time
- beaconOracle = EigenLayerBeaconOracle(0xd3D285cd1516038dAED61B8BF7Ae2daD63662492); - (bool success,) = - address(beaconOracle).call(abi.encodeWithSelector(beaconOracle.addTimestamp.selector, 1_715_918_948)); + beaconOracle = EigenLayerBeaconOracle(config.beaconOracleAddress); + (bool success,) = + address(beaconOracle).call(abi.encodeWithSelector(beaconOracle.addTimestamp.selector, block.timestamp));script/16_UpgradeExoCapsule.s.sol (1)
The review comment is correct - safety checks are necessary for the upgrade operation
The suggested safety checks are valid and align with the codebase patterns:
- Using
address(0)in ExoCapsule constructor bypasses validation in ExoCapsuleStorage- The ownership and implementation verifications follow security patterns seen in other contracts (e.g., OAppCoreUpgradeable)
- The suggested diff provides appropriate safety measures consistent with the codebase's practices
🔗 Analysis chain
Line range hint
26-31: Add safety checks for beacon upgrade operation.The upgrade operation lacks proper safety checks:
- No verification of the new implementation
- No error handling for the upgrade operation
- Initializing capsule with address(0) could be problematic
Consider adding these safety checks:
vm.startBroadcast(deployer.privateKey); console.log("owner", capsuleBeaconContract.owner()); - ExoCapsule capsule = new ExoCapsule(address(0)); + // Verify deployer is the owner + require(capsuleBeaconContract.owner() == deployer.addr, "Not authorized"); + + // Deploy new implementation with proper initialization + ExoCapsule capsule = new ExoCapsule(config.initialAddress); + + // Verify new implementation + require(address(capsule).code.length > 0, "Invalid implementation"); + + // Perform upgrade capsuleBeaconContract.upgradeTo(address(capsule)); + + // Verify upgrade + require(capsuleBeaconContract.implementation() == address(capsule), "Upgrade failed"); vm.stopBroadcast();🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find ExoCapsule contract definition ast-grep --pattern 'contract ExoCapsule { $$$ }' # Find other upgrade scripts fd -e sol -e s.sol upgrade # Find beacon contract implementation ast-grep --pattern 'contract $_ is BeaconProxy { $$$ }' # Search for upgrade patterns rg -A 5 "upgradeTo\("Length of output: 718
Script:
#!/bin/bash # Find ExoCapsule source file fd -t f -e sol ExoCapsule # Search for constructor implementation ast-grep --pattern 'constructor($$$) { $$$ }' # Look for beacon contract usage rg -l "BeaconProxy|UpgradeableBeacon" # Check initialization patterns rg -A 5 "constructor.*address.*\)"Length of output: 5893
script/19_DeployFaucet.s.sol (1)
Line range hint
41-44: Parameterize initialization values and add proxy deployment checks.The faucet deployment has several areas for improvement:
- Hardcoded initialization amount of 1 ether
- Empty proxy initialization data
- Lack of proxy deployment verification
Consider:
- Making the initialization amount configurable
- Adding proper initialization data
- Adding deployment verification checks
- faucet.initialize(exocoreValidatorSet.addr, exoEthFaucet ? tokenAddr : address(0), 1 ether); + // Get configurable amount from environment + uint256 initAmount = vm.envUint("FAUCET_INIT_AMOUNT"); + + // Prepare initialization data + bytes memory initData = abi.encodeCall( + CombinedFaucet.initialize, + (exocoreValidatorSet.addr, exoEthFaucet ? tokenAddr : address(0), initAmount) + ); + + // Deploy proxy with initialization data + TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy( + address(faucetLogic), + address(proxyAdmin), + initData + ); + + // Verify deployment + CombinedFaucet faucet = CombinedFaucet(payable(address(proxy))); + require(faucet.owner() == exocoreValidatorSet.addr, "Initialization failed");
🧹 Nitpick comments (22)
src/libraries/SignatureVerifier.sol (1)
7-64: Consider using OpenZeppelin's ECDSA library for signature verificationTo enhance security and maintainability, consider using OpenZeppelin's
ECDSAlibrary, which provides robust and well-tested functions for signature verification, including handling of Ethereum Signed Messages and support for EIP-2098 short signatures. Leveraging a widely used library can reduce potential bugs and improve code reliability.test/hardhat/integration/btc-stake-e2e.test.js (3)
38-129: Add error handling for network failures infundBitcoinAddressThe
fundBitcoinAddressfunction may encounter network errors when making API calls. Consider adding specific error handling for network failures to provide clearer feedback and retry mechanisms.
208-215: Calculate transaction fees dynamically increateStakingTransactionThe transaction fee is hardcoded as
1000satoshis. To ensure timely confirmations and adapt to network conditions, consider calculating the fee based on the transaction size and current fee rates.Apply this diff to calculate fees dynamically:
const fee = 1000; // 1000 satoshis fee +// Alternatively, calculate fee based on transaction size and fee rate +const txSize = psbt.virtualSize(); +const feeRate = await getFeeRate(); // Implement getFeeRate() to fetch current fee rates +const fee = txSize * feeRate;
289-293: Handle potential errors when funding the staker's Bitcoin addressWhen calling
fundBitcoinAddress, consider adding a try-catch block to handle any potential errors and provide informative feedback if the funding process fails.src/storage/UTXOGatewayStorage.sol (3)
471-485: Enhance validation inisValidOperatorAddressThe
isValidOperatorAddressfunction only checks the length and prefix of the address. For better security and validation, consider implementing full Bech32 address decoding and checksum verification to ensure the operator address is valid.
492-496: Consider overflow checks in_verifyInboundNonceWhile Solidity 0.8+ automatically reverts on overflows, explicitly handling potential overflows can improve code readability and safety. Consider adding a check or comment to clarify.
468-485: Visibility ofisValidOperatorAddressfunctionThe
isValidOperatorAddressfunction is marked aspublic. If it's only used internally, consider changing the visibility tointernalorprivateto reduce the contract's public interface.Apply this diff to change the function visibility:
-function isValidOperatorAddress(string calldata addressToValidate) public pure returns (bool) { +function isValidOperatorAddress(string calldata addressToValidate) internal pure returns (bool) {src/core/UTXOGateway.sol (4)
792-800: Handle unsuccessful delegation in_delegatefunctionIn the
_delegatefunction, the booleansuccessis returned but not immediately acted upon. Ensure that the calling function properly handles cases when delegation fails and provides appropriate feedback.
730-756: Add checks for potential overflows when incrementing noncesWhen incrementing
delegationNonce,pegOutNonce, andinboundNonce, consider adding checks to prevent potential overflows, even though Solidity 0.8+ handles them. This can improve code clarity and safety.
813-815: Consistent error messages in_registerAddressIn the
_registerAddressfunction, different require statements use different error messages. For consistency and clarity, consider using custom errors or consistent revert messages.Apply this diff to use consistent error handling:
-require(depositor.length > 0 && exocoreAddress != address(0), "Invalid address"); -require(inboundRegistry[clientChainId][depositor] == address(0), "Depositor address already registered"); -require(outboundRegistry[clientChainId][exocoreAddress].length == 0, "Exocore address already registered"); +if (depositor.length == 0 || exocoreAddress == address(0)) { + revert Errors.InvalidAddress(); +} +if (inboundRegistry[clientChainId][depositor] != address(0)) { + revert Errors.AddressAlreadyRegistered(); +} +if (outboundRegistry[clientChainId][exocoreAddress].length != 0) { + revert Errors.AddressAlreadyRegistered(); +}
240-250: Avoid code duplication inprocessStakeMessageThe
processStakeMessagefunction andsubmitProofForStakeMsgshare similar logic. Consider refactoring common functionality into internal functions to adhere to the DRY (Don't Repeat Yourself) principle.test/foundry/unit/UTXOGateway.t.sol (1)
488-585: Consider refactoring repeated mocking code into helper functions to improve maintainabilityThe setup of mock calls to external precompile addresses is repeated across multiple tests (e.g., lines 491-521 and 534-576). Extracting this setup into helper functions would reduce code duplication and improve readability.
src/interfaces/precompiles/IDelegation.sol (2)
80-86: Add documentation for the BTC gateway delegation method.The new method lacks documentation explaining:
- Purpose and use cases
- Parameter descriptions
- Security considerations
Add documentation similar to existing methods:
+ /// TRANSACTIONS + /// @dev delegate the client chain BTC assets to the operator through BTC gateway + /// @param clientChainId The layerZero chainID of the client chain + /// @param assetsAddress The client chain asset address + /// @param stakerAddress The staker address + /// @param operatorAddr The operator address to delegate to + /// @param opAmount The delegation amount
88-94: Add documentation for the BTC gateway undelegation method.The new method lacks documentation explaining:
- Purpose and use cases
- Parameter descriptions
- Security considerations
Add documentation similar to existing methods:
+ /// TRANSACTIONS + /// @dev undelegate the client chain BTC assets from the operator through BTC gateway + /// @param clientChainId The layerZero chainID of the client chain + /// @param assetsAddress The client chain asset address + /// @param stakerAddress The staker address + /// @param operatorAddr The operator address to undelegate from + /// @param opAmount The undelegation amountscript/hardhat/deploy-and-setup-utxogateway.script.js (1)
18-26: Consider using Promise.all for parallel transactions.The sequential distribution of ether to accounts could be optimized by using
Promise.allto execute transactions in parallel.- for (const account of initialAccounts) { - const tx = await faucet.sendTransaction({ - to: account.address, - value: ethers.parseEther("1"), - }); - await tx.wait(); - assert(await ethers.provider.getBalance(account.address) > 0, "no enough balance for account") - } + await Promise.all(initialAccounts.map(async (account) => { + const tx = await faucet.sendTransaction({ + to: account.address, + value: ethers.parseEther("1"), + }); + await tx.wait(); + assert(await ethers.provider.getBalance(account.address) > 0, "no enough balance for account") + }));test/hardhat/integration/btc-stake.test.js (2)
1-92: Test setup looks good but consider adding error handling for ETH transfers.The test setup is well-structured with clear initialization of accounts and contract deployment. However, the ETH transfer operations could benefit from error handling.
Consider adding try-catch blocks for ETH transfers:
- const tx = await faucet.sendTransaction({ - to: account.address, - value: ethers.parseEther("1"), - }); + try { + const tx = await faucet.sendTransaction({ + to: account.address, + value: ethers.parseEther("1"), + }); + await tx.wait(); + } catch (error) { + console.error(`Failed to transfer ETH to ${account.address}:`, error); + throw error; + }
141-247: Consider adding negative test cases for BTC staking.While the happy path is well tested, consider adding test cases for:
- Invalid message signatures
- Insufficient proofs
- Duplicate proof submissions
- Invalid staking amounts
Example negative test case:
it("should fail when submitting duplicate proofs", async () => { // ... setup code ... // Submit first proof await utxoGateway.connect(relayer).submitProofForStakeMsg( witness1.address, stakeMsg, proofs[0] ); // Try to submit the same proof again await expect( utxoGateway.connect(relayer).submitProofForStakeMsg( witness1.address, stakeMsg, proofs[0] ) ).to.be.revertedWith("Duplicate proof"); });src/libraries/Errors.sol (1)
317-398: LGTM! Comprehensive error handling for UTXO operations.The error definitions are well-structured and cover all critical aspects of UTXO operations, including witness management, transaction validation, and state checks.
Consider adding these additional error cases for enhanced robustness:
- Timeout-related errors for witness consensus
- Gas limit exceeded errors for complex operations
script/deployments/deployedContracts.json (1)
24-31: Ensure configuration consistency across deployment files.The UTXOGateway configuration matches the localnet deployment file but inherits the same witness configuration issue. Consider consolidating deployment configurations into a single source of truth to prevent inconsistencies.
package.json (1)
40-42: Move hardhat-gas-reporter to devDependencies.The
hardhat-gas-reporterpackage is a development tool and should be indevDependenciesinstead ofdependencies."dependencies": { "@openzeppelin/upgrades-core": "^1.40.0", - "ethers": "^6.9.0", - "hardhat-gas-reporter": "^1.0.9" + "ethers": "^6.9.0" }, "devDependencies": { + "hardhat-gas-reporter": "^1.0.9", // ... other dev dependencies }script/11_SetPeers.s.sol (2)
Line range hint
1-1: Update Solidity version to match the project configuration.The script uses Solidity 0.8.19, but the project configuration in hardhat.config.js (mentioned in the AI summary) uses 0.8.22.
-pragma solidity ^0.8.19; +pragma solidity ^0.8.22;
Line range hint
42-45: Optimize gas usage in setPeer transactions.The current implementation makes separate transactions for
setPeercalls. Consider batching these operations to reduce gas costs.Would you like me to propose an implementation for batching these operations?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (67)
.gitignore(1 hunks)hardhat.config.js(2 hunks)package.json(1 hunks)remappings.txt(0 hunks)script/10_DeployExocoreGatewayOnly.s.sol(3 hunks)script/11_SetPeers.s.sol(2 hunks)script/12_RedeployClientChainGateway.s.sol(3 hunks)script/13_DepositValidator.s.sol(2 hunks)script/14_CorrectBootstrapErrors.s.sol(4 hunks)script/15_DeploySafeMulstisigWallet.s.sol(1 hunks)script/16_UpgradeExoCapsule.s.sol(1 hunks)script/17_WithdrawalValidator.s.sol(4 hunks)script/18_SimulateReceive.s.sol(1 hunks)script/19_DeployFaucet.s.sol(1 hunks)script/1_Prerequisites.s.sol(2 hunks)script/2_DeployBoth.s.sol(2 hunks)script/3_Setup.s.sol(2 hunks)script/4_Deposit.s.sol(2 hunks)script/5_Withdraw.s.sol(2 hunks)script/6_CreateExoCapsule.s.sol(3 hunks)script/7_DeployBootstrap.s.sol(3 hunks)script/8_RegisterValidatorsAndDelegate.s.sol(1 hunks)script/9_ExtendBootstrapTime.s.sol(1 hunks)script/BaseScript.sol(1 hunks)script/TestPrecompileErrorFixed.s.sol(0 hunks)script/TestPrecompileErrorFixed_Deploy.s.sol(0 hunks)script/TokenTransfer.s.sol(0 hunks)script/deployBeaconOracle.s.sol(1 hunks)script/deployments/deployedContracts.json(1 hunks)script/deployments/utxo-gateway-exocore_localnet.json(1 hunks)script/hardhat/deploy-and-setup-utxogateway.script.js(1 hunks)slither.config.json(1 hunks)src/core/BaseRestakingController.sol(1 hunks)src/core/ClientChainGateway.sol(1 hunks)src/core/ExocoreGateway.sol(1 hunks)src/core/RewardVault.sol(1 hunks)src/core/UTXOGateway.sol(1 hunks)src/interfaces/IClientChainGateway.sol(1 hunks)src/interfaces/IExocoreGateway.sol(1 hunks)src/interfaces/precompiles/IAssets.sol(6 hunks)src/interfaces/precompiles/IDelegation.sol(1 hunks)src/libraries/Errors.sol(1 hunks)src/libraries/ExocoreBytes.sol(1 hunks)src/libraries/SignatureVerifier.sol(1 hunks)src/lzApp/OAppCoreUpgradeable.sol(1 hunks)src/lzApp/OAppReceiverUpgradeable.sol(1 hunks)src/lzApp/OAppSenderUpgradeable.sol(1 hunks)src/storage/UTXOGatewayStorage.sol(1 hunks)test/foundry/Delegation.t.sol(1 hunks)test/foundry/DepositThenDelegateTo.t.sol(1 hunks)test/foundry/DepositWithdrawPrinciple.t.sol(1 hunks)test/foundry/ExocoreDeployer.t.sol(1 hunks)test/foundry/Governance.t.sol(1 hunks)test/foundry/TvlLimits.t.sol(1 hunks)test/foundry/unit/ClientChainGateway.t.sol(1 hunks)test/foundry/unit/ExocoreGateway.t.sol(2 hunks)test/foundry/unit/UTXOGateway.t.sol(1 hunks)test/hardhat/client-chain/deploy_fixture.js(0 hunks)test/hardhat/client-chain/gateway.test.js(0 hunks)test/hardhat/exocore/deploy_fixture.js(0 hunks)test/hardhat/exocore/gateway.test.js(0 hunks)test/hardhat/integration/btc-stake-e2e.test.js(1 hunks)test/hardhat/integration/btc-stake.test.js(1 hunks)test/mocks/AssetsMock.sol(6 hunks)test/mocks/DelegationMock.sol(5 hunks)test/mocks/ExocoreGatewayMock.sol(1 hunks)test/mocks/NonShortCircuitLzEndpointMock.sol(0 hunks)
💤 Files with no reviewable changes (9)
- test/hardhat/exocore/gateway.test.js
- test/hardhat/client-chain/gateway.test.js
- script/TestPrecompileErrorFixed.s.sol
- script/TokenTransfer.s.sol
- remappings.txt
- script/TestPrecompileErrorFixed_Deploy.s.sol
- test/hardhat/client-chain/deploy_fixture.js
- test/mocks/NonShortCircuitLzEndpointMock.sol
- test/hardhat/exocore/deploy_fixture.js
✅ Files skipped from review due to trivial changes (19)
- test/foundry/DepositWithdrawPrinciple.t.sol
- .gitignore
- script/18_SimulateReceive.s.sol
- src/lzApp/OAppCoreUpgradeable.sol
- src/lzApp/OAppReceiverUpgradeable.sol
- src/lzApp/OAppSenderUpgradeable.sol
- test/foundry/TvlLimits.t.sol
- script/BaseScript.sol
- test/foundry/Governance.t.sol
- src/core/BaseRestakingController.sol
- test/foundry/ExocoreDeployer.t.sol
- test/foundry/Delegation.t.sol
- src/core/ClientChainGateway.sol
- src/interfaces/IClientChainGateway.sol
- test/foundry/DepositThenDelegateTo.t.sol
- test/foundry/unit/ExocoreGateway.t.sol
- src/interfaces/IExocoreGateway.sol
- src/core/ExocoreGateway.sol
- test/mocks/ExocoreGatewayMock.sol
🧰 Additional context used
📓 Learnings (1)
test/mocks/AssetsMock.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#23
File: test/mocks/AssetsMock.sol:9-28
Timestamp: 2024-11-12T10:03:05.536Z
Learning: The `AssetsMock` contract is used for simulating a production precompiled contract, and its complexity in data structures is intentional to mimic production behavior.
🪛 Biome (1.9.4)
hardhat.config.js
[error] 21-21: This property value named chainId is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named chainId
(lint/suspicious/noDuplicateObjectKeys)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (47)
test/mocks/AssetsMock.sol (2)
37-39: Verify staker address validation for BTC chain IDsIn the
depositLSTfunction, the staker address length check is skipped for BTC chain IDs (clientChainLzId == clientBtcChainId). Please verify that omitting this validation for BTC is intentional and does not introduce security vulnerabilities.
81-82: Verify withdrawal logic for BTC and unregistered tokensIn the
withdrawLSTfunction, the condition allows withdrawals for BTC (even if not registered) but disallows withdrawals for ETH and unregistered tokens. Please confirm that this behavior is intentional and aligns with the expected policies for token withdrawals.src/interfaces/precompiles/IAssets.sol (3)
10-45: New structsTokenInfoandStakerBalanceare well-definedThe introduction of
TokenInfoandStakerBalancestructs provides comprehensive data structures for managing token information and staker balances. The fields are appropriately defined to encapsulate necessary details.
158-162: Ensure proper access control forupdateAuthorizedGatewaysfunctionThe
updateAuthorizedGatewaysfunction allows updating authorized gateways. Please ensure that appropriate access controls are implemented to prevent unauthorized modifications to the list of authorized gateways.
180-199: Addition of query functions enhances contract interactionThe new functions
getTokenInfoandgetStakerBalanceByTokenimprove the interface by providing essential data retrieval capabilities for tokens and staker balances, facilitating better contract interaction.test/hardhat/integration/btc-stake-e2e.test.js (1)
96-99: Adjust dust threshold check for the change outputWhen calculating the change output, you check if
change > 546. The dust threshold can vary depending on the Bitcoin network and policy settings. Ensure that the dust limit is appropriate for the regtest network or make it configurable.script/9_ExtendBootstrapTime.s.sol (1)
16-16: File path updated correctlyThe file path for reading deployed contracts has been updated to match the new directory structure, ensuring the script functions properly.
hardhat.config.js (2)
20-31: Verify environment variables for local testingThe configuration now uses multiple test account private keys from environment variables. Ensure these are properly documented and included in the local development setup instructions.
Consider creating a
.env.examplefile with placeholder values for all required environment variables to help other developers set up their local environment.🧰 Tools
🪛 Biome (1.9.4)
[error] 21-21: This property value named chainId is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named chainId(lint/suspicious/noDuplicateObjectKeys)
8-8: Verify compatibility with upgraded Solidity versionThe upgrade to Solidity 0.8.22 includes breaking changes and new features. Please ensure all smart contracts, especially the UTXOGateway and BTC staking contracts, are compatible with this version.
✅ Verification successful
Solidity 0.8.22 upgrade is compatible with the codebase
All core contracts, including UTXOGateway and staking-related contracts, use ^0.8.19 which is compatible with 0.8.22. The few contracts using different versions are either non-critical components or external interfaces that won't be affected by this upgrade.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential compatibility issues in Solidity files echo "Searching for Solidity files with explicit version pragma..." fd -e sol | xargs grep -l "pragma solidity"Length of output: 3815
Script:
#!/bin/bash # Check pragma versions in core contracts echo "=== Checking pragma versions in core contracts ===" rg "pragma solidity" src/core/ -B 0 -A 1 echo -e "\n=== Checking pragma versions in interfaces ===" rg "pragma solidity" src/interfaces/ -B 0 -A 1 echo -e "\n=== Looking for potential version-specific features ===" rg "pragma solidity [\^>=]0\.8\." -l | xargs rg -l "assembly|unchecked|custom error|error [A-Z]|using.*for.*global"Length of output: 3758
script/6_CreateExoCapsule.s.sol (2)
11-12: LGTM: Updated LayerZero import pathsThe import paths have been updated to use the new
@layerzerolabsnamespace, which aligns with the library's organization.
26-26: LGTM: Improved deployment artifacts organizationMoving JSON files to the
script/deployments/directory improves the organization of deployment-related artifacts.Also applies to: 56-56
script/15_DeploySafeMulstisigWallet.s.sol (1)
27-27: LGTM: Consistent deployment artifacts locationMoving the safe contracts JSON file to
script/deployments/maintains consistency with the project's deployment file organization.src/core/RewardVault.sol (1)
28-29: LGTM: Added critical address validationThe zero-address check for the gateway parameter is a crucial security measure that prevents initialization with an invalid address.
Let's verify that similar checks exist in related contracts:
✅ Verification successful
Zero-address validation is consistently implemented
The zero-address check for the gateway parameter is consistently implemented across related contracts:
RewardVault.sol:require(gateway_ != address(0), "Gateway address cannot be zero")ExoCapsule.sol:require(gateway_ != address(0), "ExoCapsule: gateway address can not be empty")Vault.sol:if (underlyingToken_ == address(0) || gateway_ == address(0))🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for zero-address checks in gateway-related contracts ast-grep --pattern 'require($_ != address(0), $_)'Length of output: 52
Script:
#!/bin/bash # Search for various forms of zero-address validation echo "=== Searching require statements ===" rg -U "require\(.*address.*!= address\(0\).*\)" --type sol echo -e "\n=== Searching revert statements ===" rg -U "revert.*address.*zero" --type sol echo -e "\n=== Searching if conditions ===" rg -U "if.*address.*== address\(0\).*revert" --type sol echo -e "\n=== Searching custom errors ===" rg "error.*ZeroAddress" --type solLength of output: 638
Script:
#!/bin/bash # Find Solidity files first echo "=== Finding Solidity files ===" fd "\.sol$" echo -e "\n=== Searching gateway initialization and zero-address checks ===" rg -U "gateway.*=.*" -A 2 -B 2 echo -e "\n=== Searching zero-address validations ===" rg -U "require\(.*!= address\(0\).*\)" echo -e "\n=== Searching constructor and initialize patterns ===" rg -U "function (constructor|initialize).*\{" -A 5Length of output: 75015
script/10_DeployExocoreGatewayOnly.s.sol (2)
3-3: LGTM: Updated LayerZero import pathThe import path has been updated to use the new
@layerzerolabsnamespace, maintaining consistency with the library organization.
19-19: LGTM: Consistent deployment artifacts organizationJSON files have been moved to the
script/deployments/directory, maintaining a consistent structure for deployment artifacts.Also applies to: 58-58
script/1_Prerequisites.s.sol (2)
5-5: LGTM! Library update aligned with LayerZero V2 migration.The import path has been updated to use the official LayerZero Labs namespace.
76-76: Verify deployment artifacts directory exists.The JSON output path has been updated to use a
deploymentssubdirectory.✅ Verification successful
Directory structure verified successfully
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the deployments directory exists if [ ! -d "script/deployments" ]; then echo "Warning: script/deployments directory does not exist" exit 1 fiLength of output: 35
Script:
#!/bin/bash # List all directories under script/ echo "=== Directories under script/ ===" fd --type d . script/ # Check for JSON files in script/ and subdirectories echo -e "\n=== JSON files in script/ ===" fd --extension json . script/ # Look for directory creation/setup in script files echo -e "\n=== Directory setup in scripts ===" rg -l "mkdir.*deployments|mkdirp.*deployments" script/Length of output: 1191
script/5_Withdraw.s.sol (1)
11-12: LGTM! Consistent library updates.Import paths have been updated to use the official LayerZero Labs namespace, maintaining consistency across the codebase.
src/interfaces/precompiles/IDelegation.sol (1)
80-94: Verify replay protection mechanism for BTC gateway operations.The new BTC gateway methods don't include the
lzNonceparameter present in the regular delegation methods. This could affect replay protection.script/4_Deposit.s.sol (2)
10-11: LGTM! Consistent library updates.Import paths have been updated to use the official LayerZero Labs namespace, maintaining consistency across the codebase.
23-23: Verify JSON file path consistency.The JSON input path has been updated to match the new deployments directory structure.
✅ Verification successful
JSON file path is correctly structured and consistent
The file exists at
script/deployments/deployedContracts.jsonand is consistently referenced across multiple deployment scripts and tests. The path aligns with the repository's organized structure where all deployment-related JSON files are stored in thescript/deployments/directory.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the deployedContracts.json exists in the new location if [ ! -f "script/deployments/deployedContracts.json" ]; then echo "Warning: deployedContracts.json not found in script/deployments/" exit 1 fiLength of output: 58
Script:
#!/bin/bash # Check directory structure echo "=== Directory Structure ===" ls -R script/ 2>/dev/null || echo "script/ directory not found" echo -e "\n=== All JSON Files ===" fd -e json echo -e "\n=== References to deployedContracts.json ===" rg "deployedContracts.json" -lLength of output: 2909
script/8_RegisterValidatorsAndDelegate.s.sol (1)
51-51: LGTM! File path update aligns with new directory structure.The change to use the new
deploymentsdirectory is consistent with the project's reorganization.script/12_RedeployClientChainGateway.s.sol (2)
22-22: LGTM! Updated LayerZero import path.The import path has been updated to use the new
@layerzerolabsnamespace, which is the correct package.
34-34: LGTM! File paths updated to use deployments directory.File paths have been consistently updated to use the new
deploymentsdirectory structure.Also applies to: 89-89
script/13_DepositValidator.s.sol (2)
10-12: LGTM! Updated LayerZero imports.The imports have been correctly updated to use the new
@layerzerolabsnamespace.
38-39: LGTM! File paths updated to use appropriate subdirectories.Files have been organized into appropriate subdirectories:
- Validator proof moved to
data/directory- Deployed contracts moved to
deployments/directorytest/mocks/DelegationMock.sol (1)
9-9: LGTM! Mapping name change improves clarity.The rename from
delegateTotodelegateToRecordsbetter describes the purpose of the mapping.Also applies to: 44-44, 66-69, 113-113
script/hardhat/deploy-and-setup-utxogateway.script.js (1)
63-67: Verify the security implications of the required proofs parameter.The hardcoded value of 3 for
requiredProofsis a critical security parameter. Ensure this value aligns with the security requirements of the system.Run this script to check if this value is consistent across the codebase:
✅ Verification successful
The requiredProofs value of 3 is secure and properly validated
The value is protected by:
- Explicit bounds (1-10) defined in contract storage
- Validation in both initialization and updates
- Owner-only modification with pause protection
- Event emission for transparency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for requiredProofs parameter usage and its values rg -A 2 "requiredProofs"Length of output: 7697
Script:
#!/bin/bash # Search for MIN_REQUIRED_PROOFS and MAX_REQUIRED_PROOFS constants and related documentation rg -B 2 -A 2 "MIN_REQUIRED_PROOFS|MAX_REQUIRED_PROOFS" # Also check for any documentation in README or docs fd -e md | xargs rg -i "proof|witness|consensus" -A 2Length of output: 1253
script/3_Setup.s.sol (2)
12-13: LGTM! Updated import paths for LayerZero V2.The import paths have been correctly updated to use the new
@layerzerolabs/lz-evm-protocol-v2package.
24-24: LGTM! Updated file path for deployedContracts.json.The file path has been correctly updated to use the new
deploymentsdirectory structure.script/14_CorrectBootstrapErrors.s.sol (2)
18-18: LGTM! Updated import path for LayerZero V2.The import path has been correctly updated to use the new
@layerzerolabs/lz-evm-protocol-v2package.
40-40: LGTM! Updated file paths for JSON files.The file paths have been correctly updated to use the new
deploymentsdirectory structure:
- prerequisiteContracts.json
- deployedBootstrapOnly.json
- correctBootstrapErrors.json
Also applies to: 59-59, 139-139
script/7_DeployBootstrap.s.sol (2)
17-17: LGTM! Updated import path for LayerZero V2.The import path has been correctly updated to use the new
@layerzerolabs/lz-evm-protocol-v2package.
32-32: LGTM! Updated file paths for JSON files.The file paths have been correctly updated to use the new
deploymentsdirectory structure:
- prerequisiteContracts.json
- deployedBootstrapOnly.json
Also applies to: 150-150
script/17_WithdrawalValidator.s.sol (2)
12-13: LGTM! Library imports updated to use the new LayerZero package.The imports have been correctly updated to use
@layerzerolabs/lz-evm-protocol-v2instead of the older@layerzero-v2package.
45-45: LGTM! JSON file paths updated for better organization.The file paths have been correctly updated to use the new directory structure:
- Deployment data moved to
script/deployments/- Withdrawal proof data moved to
script/data/Also applies to: 98-98, 105-105, 121-121, 128-128
script/2_DeployBoth.s.sol (2)
17-17: LGTM! LayerZero endpoint import updated.The import has been correctly updated to use
@layerzerolabs/lz-evm-protocol-v2.
28-28: LGTM! Prerequisites file path updated.The file path has been correctly updated to use the new
script/deployments/directory.test/hardhat/integration/btc-stake.test.js (2)
94-114: LGTM! Comprehensive deployment verification.The test thoroughly verifies:
- Contract deployment
- Owner settings
- Witness authorization
- Threshold configuration
- Gateway authorization
116-139: LGTM! Thorough BTC staking activation test.The test properly verifies:
- Staking activation
- Client chain registration
- Token registration with correct parameters
test/foundry/unit/ClientChainGateway.t.sol (1)
5-5: LGTM! LayerZero imports updated consistently.The imports have been correctly updated to use
@layerzerolabs/lz-evm-protocol-v2package, maintaining consistency with other files.Also applies to: 7-8
slither.config.json (1)
2-2: Reconsider excluding the mapping-deletion detector.Excluding the mapping-deletion detector from Slither analysis could hide potential issues with improper storage cleanup in mappings, which is particularly important for upgradeable contracts like UTXOGateway.
package.json (1)
18-37: Review dependency version constraints.Several dependencies lack version constraints or use potentially unstable versions:
@evmos/evmosjsis pinned to a git branch (main) which could lead to unexpected breaking changeshardhatv2.22.17 is very recent and might have stability issuesPlease verify the stability of these versions by running:
script/deployBeaconOracle.s.sol (1)
5-5: LGTM! Import path updated to use LayerZero Labs namespace.The import path has been correctly updated to use the new LayerZero Labs namespace.
script/16_UpgradeExoCapsule.s.sol (1)
15-15: LGTM! JSON file path updated to new deployments directory.The path has been correctly updated to use the new standardized deployments directory structure.
script/19_DeployFaucet.s.sol (2)
19-19: LGTM! JSON file path updated to new deployments directory.The path has been correctly updated to use the new standardized deployments directory structure.
Line range hint
45-49: Consider storing deployment addresses securely.The comment indicates addresses are intentionally kept private, but console logging them might expose them in logs.
Consider:
- Using a secure logging mechanism
- Encrypting sensitive data before logging
- Implementing proper access controls for deployment logs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/storage/UTXOGatewayStorage.sol (1)
481-489: Simplify prefix check usingbytes.startsWithIn the
isValidOperatorAddressfunction, you manually check if the address starts with the prefix by looping through each character. You can simplify this by using thebytes.startsWithfunction available in Solidity 0.8.4 and above.Apply this diff to simplify the prefix check:
function isValidOperatorAddress(string calldata addressToValidate) public pure returns (bool) { bytes memory stringBytes = bytes(addressToValidate); if (stringBytes.length != 42) { return false; } - for (uint256 i = 0; i < EXO_ADDRESS_PREFIX.length; ++i) { - if (stringBytes[i] != EXO_ADDRESS_PREFIX[i]) { - return false; - } - } + if (!stringBytes.startsWith(EXO_ADDRESS_PREFIX)) { + return false; + } return true; }test/hardhat/integration/btc-stake-e2e.test.js (1)
149-268: Consider making hardcoded values configurable.The implementation is solid, but consider extracting these hardcoded values to configuration:
- Required balance (0.01 BTC)
- Transaction fee (1000 satoshis)
- Dust threshold (546 satoshis)
+// At the top of the file +const CONFIG = { + REQUIRED_BALANCE: 0.01, // BTC + TX_FEE: 1000, // satoshis + DUST_THRESHOLD: 546 // satoshis +}; async function createStakingTransaction(stakerPrivateKey, vaultAddress, depositAmount) { // ... - const requiredBalance = 0.01; + const requiredBalance = CONFIG.REQUIRED_BALANCE; // ... - const fee = 1000; + const fee = CONFIG.TX_FEE; // ... - if (change > 546) { + if (change > CONFIG.DUST_THRESHOLD) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
package.json(1 hunks)script/deployments/deployedContracts.json(1 hunks)script/deployments/utxo-gateway-exocore_localnet.json(1 hunks)src/core/UTXOGateway.sol(1 hunks)src/libraries/Errors.sol(1 hunks)src/storage/UTXOGatewayStorage.sol(1 hunks)test/foundry/unit/UTXOGateway.t.sol(1 hunks)test/hardhat/integration/btc-stake-e2e.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- script/deployments/utxo-gateway-exocore_localnet.json
- script/deployments/deployedContracts.json
- test/foundry/unit/UTXOGateway.t.sol
🧰 Additional context used
📓 Learnings (2)
test/hardhat/integration/btc-stake-e2e.test.js (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/hardhat/integration/btc-stake-e2e.test.js:24-36
Timestamp: 2025-01-08T03:12:49.494Z
Learning: In Bitcoin transaction tests, prefer using test-level timeouts over function-level timeouts to handle long-running operations, as it provides a single point of control and avoids redundant timeout handling.
src/core/UTXOGateway.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/core/UTXOGateway.sol:407-424
Timestamp: 2025-01-08T03:15:08.317Z
Learning: The `onlyAuthorizedWitness` modifier checking if `msg.sender` is an authorized witness provides sufficient access control for functions that should only be called by witnesses in the UTXOGateway contract.
🪛 Biome (1.9.4)
test/hardhat/integration/btc-stake-e2e.test.js
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (7)
src/core/UTXOGateway.sol (2)
406-423: Ensure proper authorization checks inmarkPegOutRequestProcessedThe
markPegOutRequestProcessedfunction relies ononlyAuthorizedWitnessfor access control. Verify that this modifier adequately restricts access and consider additional checks if necessary to prevent unauthorized calls.
861-867: Handle potential failure in_delegatecallIn the
_processStakeMsgfunction, if the_delegatecall fails, you emit aDelegationFailedForStakeevent but continue execution. Consider whether additional actions are necessary to handle the failure and maintain consistency in the system state.Do you want to review the error handling strategy for delegation failures to ensure system integrity?
package.json (1)
12-12: Update the test script to run the test suiteThe current test script exits with an error message. Since this PR focuses on adding unit tests for BTC staking, the test script should run the actual tests.
Apply this diff to fix the test script:
"scripts": { - "test": "echo \"Error: no test specified\" && exit 1", + "test": "hardhat test", "deploy:utxogateway": "hardhat run script/hardhat/deploy-and-setup-utxogateway.script.js --network exocore_localnet" },test/hardhat/integration/btc-stake-e2e.test.js (3)
25-54: LGTM! The implementation aligns with the test-level timeout preference.The function correctly implements transaction confirmation checking with appropriate error handling and a reasonable polling interval. The while(true) loop is acceptable here as it's controlled by the test-level timeout.
🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
56-147: LGTM! Comprehensive implementation with proper UTXO management.The function includes all necessary components:
- Input validation
- Proper UTXO selection and fee calculation
- Dust threshold check for change outputs
- Transaction confirmation verification
- Error handling
342-382: 🛠️ Refactor suggestionAdd event listener cleanup to prevent memory leaks.
The event listener should be removed after the test completes or if an error occurs to prevent memory leaks in subsequent tests.
utxoGateway.on('DepositCompleted', async ( clientChainId, txTag, depositorExoAddr, depositorClientChainAddr, amount, updatedBalance ) => { try { if (depositorExoAddr.toLowerCase() === staker.address.toLowerCase()) { clearTimeout(timeout); + utxoGateway.removeAllListeners('DepositCompleted'); // ... rest of the code ... resolve(); } } catch (error) { clearTimeout(timeout); + utxoGateway.removeAllListeners('DepositCompleted'); reject(error); } });Likely invalid or redundant comment.
src/libraries/Errors.sol (1)
317-401: LGTM! Comprehensive and well-documented error definitions.The UTXOGateway errors are:
- Well-documented with clear descriptions
- Follow consistent naming conventions
- Include appropriate parameters for debugging
- Cover all necessary failure scenarios
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
package.json (1)
41-41: Consider moving hardhat-gas-reporter to devDependencies.
hardhat-gas-reporteris a development tool and should be indevDependenciesrather thandependencies."dependencies": { "@openzeppelin/upgrades-core": "^1.40.0", - "ethers": "^6.9.0", - "hardhat-gas-reporter": "^1.0.9" + "ethers": "^6.9.0" }, "devDependencies": { + "hardhat-gas-reporter": "^1.0.9", // ... other dev dependenciestest/hardhat/integration/btc-stake-e2e.test.js (3)
21-23: Improve error message clarity.The error message doesn't match all the environment variables being checked.
- throw new Error('BITCOIN_ESPLORA_API_URL or TEST_ACCOUNT_THREE_PRIVATE_KEY or BITCOIN_VAULT_ADDRESS is not set'); + throw new Error('One or more required environment variables are not set: BITCOIN_ESPLORA_API_URL, TEST_ACCOUNT_THREE_PRIVATE_KEY, BITCOIN_VAULT_ADDRESS, BITCOIN_STAKER_PRIVATE_KEY');
149-268: Consider extracting transaction building logic.The
createStakingTransactionfunction is quite long and handles multiple responsibilities. Consider extracting the transaction building logic into a separate function for better maintainability.+async function buildBitcoinTransaction(psbt, inputs, outputs, keyPair) { + // Add inputs + for (const input of inputs) { + psbt.addInput(input); + } + + // Add outputs + for (const output of outputs) { + psbt.addOutput(output); + } + + // Sign and finalize + psbt.signAllInputs(keyPair); + psbt.finalizeAllInputs(); + + return psbt.extractTransaction(); +}
340-391: Consider using async/await pattern instead of Promise constructor.The deposit processing check could be simplified using an async/await pattern with a timeout wrapper function.
- return new Promise((resolve, reject) => { - const timeout = setTimeout(() => { - reject(new Error('Timeout waiting for deposit to be processed')); - }, 120000); + const waitWithTimeout = async (timeoutMs) => { + const startTime = Date.now(); + while (Date.now() - startTime < timeoutMs) { + const isProcessed = await utxoGateway.isStakeMsgProcessed( + CLIENT_CHAIN.BTC, + ethers.getBytes('0x' + txid) + ); + + if (isProcessed) { + return true; + } + await new Promise(resolve => setTimeout(resolve, 1000)); + } + throw new Error('Timeout waiting for deposit to be processed'); + }; + + await waitWithTimeout(120000);src/core/UTXOGateway.sol (4)
228-249: Ensure single consensus path inprocessStakeMessageThe function
processStakeMessageprocesses a stake message when consensus is not required. However, if consensus is required, it reverts. This logic is also present insubmitProofForStakeMsg, which handles processing when consensus is required.Consider refactoring to ensure there is a single path for processing stake messages, reducing code duplication and potential inconsistencies.
Apply this diff to refactor the logic:
function processStakeMessage(address witness, StakeMsg calldata _msg, bytes calldata signature) external nonReentrant whenNotPaused + onlyWhenConsensusNotRequired { if (_isConsensusRequired()) { revert Errors.ConsensusRequired(); } if (!_isAuthorizedWitness(witness)) { revert Errors.WitnessNotAuthorized(witness); } _verifyStakeMessage(witness, _msg, signature); _processStakeMsg(_msg); }And add the modifier:
+modifier onlyWhenConsensusNotRequired() { + if (_isConsensusRequired()) { + revert Errors.ConsensusRequired(); + } + _; +}
684-698: Validate operator address format in_verifyStakeMsgFieldsThe function
_verifyStakeMsgFieldschecks if the operator address is valid by verifying its length. However, additional validation can be performed to ensure the operator address follows the expected format, especially if it's used elsewhere in the system.Consider enhancing the validation logic:
function _verifyStakeMsgFields(StakeMsg calldata _msg) internal pure { if ( uint8(_msg.clientChainId) == 0 || _msg.nonce == 0 || uint256(_msg.clientTxId) == 0 || _msg.clientAddress.length == 0 || _msg.amount == 0 ) { revert Errors.InvalidStakeMessage(); } if (bytes(_msg.operator).length > 0 && !isValidOperatorAddress(_msg.operator)) { revert Errors.InvalidOperator(); + } else if (bytes(_msg.operator).length == 0) { + revert Errors.OperatorAddressRequired(); } }
318-319: Avoid using deprecatedrequirefunctionThe check for
clientAddress.length == 0usesrevert Errors.AddressNotRegistered();, which is appropriate. However, in other parts of the code,requireis used without specifying a custom error.Consider consistently using custom errors for revert statements to save gas and maintain consistency.
Apply this diff:
function withdrawPrincipal(Token token, uint256 amount) external nonReentrant whenNotPaused isValidAmount(amount) { ClientChainID clientChainId = ClientChainID(uint8(token)); bytes memory clientAddress = outboundRegistry[clientChainId][msg.sender]; if (clientAddress.length == 0) { revert Errors.AddressNotRegistered(); } - require(clientAddress.length > 0, "Address not registered");
791-808: Handle delegation failure appropriately in_delegateThe
_delegatefunction returns a boolean indicating success or failure. When delegation fails, consider emitting an event or reverting, depending on the desired behavior.Ensure that the calling functions handle the return value correctly and that failure scenarios are adequately covered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
hardhat.config.js(2 hunks)package.json(1 hunks)script/2_DeployBoth.s.sol(3 hunks)script/deployments/utxo-gateway-exocore_localnet.json(1 hunks)src/core/UTXOGateway.sol(1 hunks)src/libraries/SignatureVerifier.sol(1 hunks)test/hardhat/integration/btc-stake-e2e.test.js(1 hunks)test/mocks/DelegationMock.sol(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- script/2_DeployBoth.s.sol
- script/deployments/utxo-gateway-exocore_localnet.json
- hardhat.config.js
- test/mocks/DelegationMock.sol
🧰 Additional context used
📓 Learnings (2)
src/core/UTXOGateway.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/core/UTXOGateway.sol:407-424
Timestamp: 2025-01-08T03:15:08.317Z
Learning: The `onlyAuthorizedWitness` modifier checking if `msg.sender` is an authorized witness provides sufficient access control for functions that should only be called by witnesses in the UTXOGateway contract.
test/hardhat/integration/btc-stake-e2e.test.js (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/hardhat/integration/btc-stake-e2e.test.js:24-36
Timestamp: 2025-01-08T03:12:49.494Z
Learning: In Bitcoin transaction tests, prefer using test-level timeouts over function-level timeouts to handle long-running operations, as it provides a single point of control and avoids redundant timeout handling.
🪛 Biome (1.9.4)
test/hardhat/integration/btc-stake-e2e.test.js
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (7)
package.json (2)
12-13: LGTM! Test script has been updated.The test script has been correctly updated to use Hardhat for testing.
26-36: Verify compatibility of Bitcoin-related dependencies.The Bitcoin-related dependencies (
@cosmjs/proto-signing,@cosmjs/stargate,bitcoinjs-lib, etc.) should be verified for compatibility with the Bitcoin network being used (mainnet/testnet/regtest).✅ Verification successful
Bitcoin-related dependencies are properly configured and compatible
The dependencies are correctly set up with explicit network configuration (regtest) in test environments. The versions of
bitcoinjs-libandtiny-secp256k1are compatible, and their Node.js engine requirements are properly aligned.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the installed versions of Bitcoin-related packages are compatible jq -r '.dependencies,.devDependencies | to_entries[] | select(.key | contains("bitcoin") or contains("secp256k1")) | "\(.key)@\(.value)"' package.json | while read pkg; do npm view "$pkg" engines doneLength of output: 411
Script:
#!/bin/bash # Search for Bitcoin network configuration and initialization echo "=== Searching for Bitcoin network configuration ===" rg -i "bitcoin.*network|testnet|mainnet|regtest" -g '!package*.json' -g '!*.lock' echo -e "\n=== Searching for bitcoinjs-lib usage ===" ast-grep --pattern 'require("bitcoinjs-lib")' || ast-grep --pattern 'from "bitcoinjs-lib"' echo -e "\n=== Checking for potential version conflicts ===" rg -i "secp256k1|bitcoinjs" -g 'package*.json'Length of output: 5711
test/hardhat/integration/btc-stake-e2e.test.js (2)
25-54: LGTM! Timeout handling is appropriately implemented.The function uses test-level timeout as recommended, which aligns with the retrieved learning from previous discussions.
🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
314-392: LGTM! Test implementation follows best practices.The test case:
- Uses appropriate timeouts at the test level
- Includes proper balance verification
- Handles asynchronous operations correctly
- Includes comprehensive error handling
src/libraries/SignatureVerifier.sol (1)
7-69: LGTM!The
SignatureVerifierlibrary correctly implements signature verification for both 64-byte (EIP-2098) and 65-byte signatures, with proper error handling and efficient use of inline assembly.src/core/UTXOGateway.sol (2)
184-226: Mitigate potential race conditions insubmitProofForStakeMsgIn the
submitProofForStakeMsgfunction, there may be potential race conditions when multiple witnesses are submitting proofs simultaneously. Consider adding mechanisms to handle concurrent submissions gracefully.Please review the logic and ensure that state changes are properly managed under concurrent access. If needed, simulate concurrent submissions in tests to verify correct behavior.
94-103:⚠️ Potential issueValidate
clientChainIdto prevent invalid enum valuesCurrently, functions like
activateStakingForClientChaindo not validate whether the providedclientChainIdcorresponds to a validClientChainID. Since enums in Solidity can be cast from anyuint8value, there's a risk that invalid or unexpected values may be passed, potentially leading to unintended behavior.Apply this diff to add validation for
clientChainId:function activateStakingForClientChain(ClientChainID clientChainId) external onlyOwner whenNotPaused { + if (clientChainId != ClientChainID.BITCOIN) { + revert Errors.InvalidClientChain(); + } if (clientChainId == ClientChainID.BITCOIN) { _registerOrUpdateClientChain( clientChainId, STAKER_ACCOUNT_LENGTH, BITCOIN_NAME, BITCOIN_METADATA, BITCOIN_SIGNATURE_SCHEME ); _registerOrUpdateToken(clientChainId, VIRTUAL_TOKEN, BTC_DECIMALS, BTC_NAME, BTC_METADATA, BTC_ORACLE_INFO); } else { - revert Errors.InvalidClientChain(); } }Alternatively, consider adding a modifier to validate
clientChainIdacross all functions that accept it:+modifier isValidClientChainId(ClientChainID clientChainId) { + if (clientChainId != ClientChainID.BITCOIN) { + revert Errors.InvalidClientChain(); + } + _; +} function activateStakingForClientChain(ClientChainID clientChainId) external onlyOwner whenNotPaused + isValidClientChainId(clientChainId) { // Function implementation... }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (15)
src/libraries/SignatureVerifier.sol (2)
27-29: Inline Variable Declarations for Gas OptimizationYou can declare
r,s, andvwithout initialization to optimize gas usage.Apply this diff:
function verifyMsgSig(address signer, bytes32 messageHash, bytes memory signature) internal pure { - // Declare r, s, and v signature parameters. - bytes32 r = 0; - bytes32 s = 0; - uint8 v = 0; + bytes32 r; + bytes32 s; + uint8 v;
Line range hint
68-79: Implement EIP-712 for Structured Data SigningThe current message hashing method lacks domain separation and might be susceptible to replay attacks across different contracts or chains. Consider using EIP-712 for hashing and signing structured data to enhance security.
src/storage/UTXOGatewayStorage.sol (1)
475-491: Enhance Address Validation LogicIn the
isValidOperatorAddressfunction, the current validation checks if the address length is 42 and starts with "exo1". This may not be sufficient to ensure the address is valid.Consider using a Bech32 library or implementing additional checksum validation to ensure the operator address is correctly formatted and valid.
src/core/UTXOGateway.sol (4)
680-681: Use EIP-712 for Secure Message SigningIn
_verifySignature, the message hash is computed without domain separation. Implementing EIP-712 structured data hashing will add domain separation and prevent replay attacks.
810-815: Emit Events After State ChangesIn
_revokeTxIfExpired, the eventTransactionExpiredis emitted after changing the transaction status. It's best practice to emit events after state changes to reflect the new state accurately.Apply this diff:
function _revokeTxIfExpired(bytes32 txid) internal { Transaction storage txn = transactions[txid]; if (txn.status == TxStatus.PENDING && block.timestamp >= txn.expiryTime) { txn.status = TxStatus.EXPIRED; + emit TransactionExpired(txid); } - emit TransactionExpired(txid); }
818-827: Replacerequirewith Custom Errors for Gas EfficiencyIn
_registerAddress, using custom errors instead ofrequirestatements saves gas and maintains consistency.Apply this diff:
function _registerAddress(ClientChainID clientChainId, bytes memory depositor, address exocoreAddress) internal { - require(depositor.length > 0 && exocoreAddress != address(0), "Invalid address"); - require(inboundRegistry[clientChainId][depositor] == address(0), "Depositor address already registered"); - require(outboundRegistry[clientChainId][exocoreAddress].length == 0, "Exocore address already registered"); + if (depositor.length == 0 || exocoreAddress == address(0)) { + revert Errors.InvalidAddress(); + } + if (inboundRegistry[clientChainId][depositor] != address(0)) { + revert Errors.DepositorAddressAlreadyRegistered(); + } + if (outboundRegistry[clientChainId][exocoreAddress].length != 0) { + revert Errors.ExocoreAddressAlreadyRegistered(); + }Add the following custom errors to your
Errorslibrary:error InvalidAddress(); error DepositorAddressAlreadyRegistered(); error ExocoreAddressAlreadyRegistered();
774-789: Handle Deposit Failure GracefullyIn
_deposit, if the deposit fails, the function reverts withDepositFailed. Consider providing more detailed error information to aid in debugging.Update the revert statement to include the
depositorExoAddror other relevant details.if (!success) { - revert Errors.DepositFailed(clientTxId); + revert Errors.DepositFailed(clientTxId, depositorExoAddr); }Update the
Errorslibrary accordingly.package.json (1)
18-37: Consider adding scripts for security auditing and gas optimization.While the dependencies are comprehensive, consider adding tools for security analysis and gas optimization.
Add the following development dependencies:
"devDependencies": { + "slither-analyzer": "^0.9.3", + "solidity-coverage": "^0.8.0", ... }Also applies to: 41-41
test/mocks/AssetsMock.sol (1)
37-50: Enhance validation in depositLST function.The deposit logic could be improved with more explicit validation and error messages.
Consider this enhancement:
- if (clientChainLzId != clientBtcChainId) { - require(stakerAddress.length == 32, "invalid staker address"); - } + if (clientChainLzId != clientBtcChainId) { + require(stakerAddress.length == 32, "invalid staker address length for non-BTC chain"); + } else { + require(stakerAddress.length > 0, "empty staker address for BTC chain"); + }src/interfaces/precompiles/IAssets.sol (1)
158-162: Consider adding validation parameters to updateAuthorizedGateways.The function could benefit from additional parameters for validation.
Consider adding a version parameter to prevent replay attacks:
- function updateAuthorizedGateways(address[] calldata gateways) external returns (bool success); + function updateAuthorizedGateways( + address[] calldata gateways, + uint256 version + ) external returns (bool success);test/hardhat/integration/btc-stake-e2e.test.js (5)
21-23: Update error message to include all checked environment variables.The error message doesn't mention
BITCOIN_STAKER_PRIVATE_KEYwhich is also being checked.- throw new Error('BITCOIN_ESPLORA_API_URL or TEST_ACCOUNT_THREE_PRIVATE_KEY or BITCOIN_VAULT_ADDRESS is not set'); + throw new Error('One or more required environment variables are not set: BITCOIN_ESPLORA_API_URL, TEST_ACCOUNT_THREE_PRIVATE_KEY, BITCOIN_VAULT_ADDRESS, BITCOIN_STAKER_PRIVATE_KEY');
33-34: Use optional chaining for safer property access.The
statusproperty might be undefined, which could cause runtime errors.- if (tx.status && tx.status.confirmed) { + if (tx.status?.confirmed) {🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
115-115: Consider making the transaction fee configurable.The hardcoded fee of 1000 satoshis might not be optimal for all network conditions. Consider making it a parameter with a default value.
- const fee = 1000; + const fee = options?.fee ?? 1000; // Default to 1000 satoshis if not specified
181-181: Make the required balance configurable.The hardcoded balance requirement of 0.01 BTC might not be suitable for all test scenarios.
- const requiredBalance = 0.01; // set required balance to 0.01 BTC + const requiredBalance = depositAmount * 1.5; // Set required balance to 150% of deposit amount for fees
372-373: Document the meaning of balance array indices.The use of magic numbers (3 and 7) for array indices makes the code less maintainable. Consider using named constants or adding comments to explain their meaning.
+ // Balance array indices from IAssets precompile + const BALANCE_AMOUNT_INDEX = 3; + const TOTAL_BALANCE_INDEX = 7; + - expect(finalBalance[3] - (initialBalance ? initialBalance[3] : 0)).to.equal(expectedIncrease); - expect(finalBalance[7] - (initialBalance ? initialBalance[7] : 0)).to.equal(expectedIncrease); + expect(finalBalance[BALANCE_AMOUNT_INDEX] - (initialBalance ? initialBalance[BALANCE_AMOUNT_INDEX] : 0)) + .to.equal(expectedIncrease); + expect(finalBalance[TOTAL_BALANCE_INDEX] - (initialBalance ? initialBalance[TOTAL_BALANCE_INDEX] : 0)) + .to.equal(expectedIncrease);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (69)
.gitignore(1 hunks)hardhat.config.js(2 hunks)package.json(1 hunks)remappings.txt(0 hunks)script/10_DeployExocoreGatewayOnly.s.sol(3 hunks)script/11_SetPeers.s.sol(2 hunks)script/12_RedeployClientChainGateway.s.sol(3 hunks)script/13_DepositValidator.s.sol(2 hunks)script/14_CorrectBootstrapErrors.s.sol(4 hunks)script/15_DeploySafeMulstisigWallet.s.sol(1 hunks)script/16_UpgradeExoCapsule.s.sol(1 hunks)script/17_WithdrawalValidator.s.sol(4 hunks)script/18_SimulateReceive.s.sol(1 hunks)script/19_DeployFaucet.s.sol(1 hunks)script/1_Prerequisites.s.sol(2 hunks)script/2_DeployBoth.s.sol(3 hunks)script/3_Setup.s.sol(2 hunks)script/4_Deposit.s.sol(2 hunks)script/5_Withdraw.s.sol(2 hunks)script/6_CreateExoCapsule.s.sol(3 hunks)script/7_DeployBootstrap.s.sol(3 hunks)script/8_RegisterValidatorsAndDelegate.s.sol(1 hunks)script/9_ExtendBootstrapTime.s.sol(1 hunks)script/BaseScript.sol(1 hunks)script/TestPrecompileErrorFixed.s.sol(0 hunks)script/TestPrecompileErrorFixed_Deploy.s.sol(0 hunks)script/TokenTransfer.s.sol(0 hunks)script/deployBeaconOracle.s.sol(1 hunks)script/deployments/deployedContracts.json(1 hunks)script/deployments/utxo-gateway-exocore_localnet.json(1 hunks)script/hardhat/deploy-and-setup-utxogateway.script.js(1 hunks)slither.config.json(1 hunks)src/core/BaseRestakingController.sol(1 hunks)src/core/ClientChainGateway.sol(1 hunks)src/core/ExocoreGateway.sol(1 hunks)src/core/RewardVault.sol(1 hunks)src/core/UTXOGateway.sol(1 hunks)src/interfaces/IClientChainGateway.sol(1 hunks)src/interfaces/IExocoreGateway.sol(1 hunks)src/interfaces/precompiles/IAssets.sol(6 hunks)src/libraries/Errors.sol(1 hunks)src/libraries/ExocoreBytes.sol(1 hunks)src/libraries/SignatureVerifier.sol(1 hunks)src/lzApp/OAppCoreUpgradeable.sol(1 hunks)src/lzApp/OAppReceiverUpgradeable.sol(1 hunks)src/lzApp/OAppSenderUpgradeable.sol(1 hunks)src/storage/UTXOGatewayStorage.sol(1 hunks)test/foundry/BootstrapDepositNST.t.sol(1 hunks)test/foundry/Delegation.t.sol(1 hunks)test/foundry/DepositThenDelegateTo.t.sol(1 hunks)test/foundry/DepositWithdrawPrinciple.t.sol(1 hunks)test/foundry/ExocoreDeployer.t.sol(1 hunks)test/foundry/Governance.t.sol(1 hunks)test/foundry/TvlLimits.t.sol(1 hunks)test/foundry/unit/Bootstrap.t.sol(1 hunks)test/foundry/unit/ClientChainGateway.t.sol(1 hunks)test/foundry/unit/ExoCapsule.t.sol(2 hunks)test/foundry/unit/ExocoreGateway.t.sol(2 hunks)test/foundry/unit/UTXOGateway.t.sol(1 hunks)test/hardhat/client-chain/deploy_fixture.js(0 hunks)test/hardhat/client-chain/gateway.test.js(0 hunks)test/hardhat/exocore/deploy_fixture.js(0 hunks)test/hardhat/exocore/gateway.test.js(0 hunks)test/hardhat/integration/btc-stake-e2e.test.js(1 hunks)test/hardhat/integration/btc-stake.test.js(1 hunks)test/mocks/AssetsMock.sol(6 hunks)test/mocks/DelegationMock.sol(4 hunks)test/mocks/ExocoreGatewayMock.sol(1 hunks)test/mocks/NonShortCircuitLzEndpointMock.sol(0 hunks)
💤 Files with no reviewable changes (9)
- test/hardhat/exocore/gateway.test.js
- remappings.txt
- test/hardhat/client-chain/gateway.test.js
- test/mocks/NonShortCircuitLzEndpointMock.sol
- script/TestPrecompileErrorFixed_Deploy.s.sol
- script/TestPrecompileErrorFixed.s.sol
- script/TokenTransfer.s.sol
- test/hardhat/client-chain/deploy_fixture.js
- test/hardhat/exocore/deploy_fixture.js
🚧 Files skipped from review as they are similar to previous changes (48)
- .gitignore
- src/libraries/ExocoreBytes.sol
- script/11_SetPeers.s.sol
- script/9_ExtendBootstrapTime.s.sol
- test/foundry/DepositWithdrawPrinciple.t.sol
- script/3_Setup.s.sol
- script/16_UpgradeExoCapsule.s.sol
- test/foundry/TvlLimits.t.sol
- script/12_RedeployClientChainGateway.s.sol
- script/BaseScript.sol
- script/4_Deposit.s.sol
- src/core/BaseRestakingController.sol
- script/8_RegisterValidatorsAndDelegate.s.sol
- script/5_Withdraw.s.sol
- script/7_DeployBootstrap.s.sol
- src/lzApp/OAppReceiverUpgradeable.sol
- test/foundry/ExocoreDeployer.t.sol
- src/core/RewardVault.sol
- script/15_DeploySafeMulstisigWallet.s.sol
- script/2_DeployBoth.s.sol
- slither.config.json
- src/interfaces/IClientChainGateway.sol
- script/6_CreateExoCapsule.s.sol
- test/foundry/DepositThenDelegateTo.t.sol
- test/foundry/Governance.t.sol
- src/lzApp/OAppCoreUpgradeable.sol
- script/10_DeployExocoreGatewayOnly.s.sol
- script/deployBeaconOracle.s.sol
- script/18_SimulateReceive.s.sol
- script/14_CorrectBootstrapErrors.s.sol
- src/core/ClientChainGateway.sol
- script/17_WithdrawalValidator.s.sol
- script/19_DeployFaucet.s.sol
- test/foundry/Delegation.t.sol
- src/interfaces/IExocoreGateway.sol
- script/deployments/deployedContracts.json
- script/deployments/utxo-gateway-exocore_localnet.json
- src/lzApp/OAppSenderUpgradeable.sol
- test/hardhat/integration/btc-stake.test.js
- script/1_Prerequisites.s.sol
- script/13_DepositValidator.s.sol
- test/foundry/unit/ExocoreGateway.t.sol
- test/mocks/ExocoreGatewayMock.sol
- script/hardhat/deploy-and-setup-utxogateway.script.js
- src/core/ExocoreGateway.sol
- test/mocks/DelegationMock.sol
- test/foundry/unit/UTXOGateway.t.sol
- test/foundry/unit/ClientChainGateway.t.sol
🧰 Additional context used
📓 Learnings (5)
test/foundry/BootstrapDepositNST.t.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#121
File: test/foundry/BootstrapDepositNST.t.sol:285-300
Timestamp: 2024-11-12T10:03:05.536Z
Learning: In `test/foundry/BootstrapDepositNST.t.sol`, adu-web3 prefers to use `vm.etch` and direct storage manipulation in tests, instead of deploying the capsule contract with the desired state or using Solidity inheritance and overrides.
test/mocks/AssetsMock.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#23
File: test/mocks/AssetsMock.sol:9-28
Timestamp: 2024-11-12T10:03:05.536Z
Learning: The `AssetsMock` contract is used for simulating a production precompiled contract, and its complexity in data structures is intentional to mimic production behavior.
src/storage/UTXOGatewayStorage.sol (2)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/storage/UTXOGatewayStorage.sol:457-462
Timestamp: 2025-01-20T02:28:24.440Z
Learning: In the UTXOGateway contract, Token.NONE cannot be registered in outboundRegistry because registration requires a valid ClientChainID and goes through _registerAddress function which has multiple validations. The clientChainId parameter comes from validated sources (stake messages, etc.), making additional Token.NONE checks in the isRegistered modifier redundant.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/storage/UTXOGatewayStorage.sol:457-462
Timestamp: 2025-01-20T02:28:24.440Z
Learning: In the UTXOGateway contract, Token.NONE cannot be registered in outboundRegistry due to validations during the registration process, making additional Token.NONE checks in the isRegistered modifier redundant.
src/core/UTXOGateway.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/core/UTXOGateway.sol:407-424
Timestamp: 2025-01-08T03:15:08.317Z
Learning: The `onlyAuthorizedWitness` modifier checking if `msg.sender` is an authorized witness provides sufficient access control for functions that should only be called by witnesses in the UTXOGateway contract.
test/hardhat/integration/btc-stake-e2e.test.js (2)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/hardhat/integration/btc-stake-e2e.test.js:0-0
Timestamp: 2025-01-20T02:24:57.006Z
Learning: In the Exocore project's test suite, prefer using interval-based contract state querying over event listeners for monitoring state changes, as it provides better control and reliability.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/hardhat/integration/btc-stake-e2e.test.js:24-36
Timestamp: 2025-01-08T03:12:49.494Z
Learning: In Bitcoin transaction tests, prefer using test-level timeouts over function-level timeouts to handle long-running operations, as it provides a single point of control and avoids redundant timeout handling.
🪛 Biome (1.9.4)
test/hardhat/integration/btc-stake-e2e.test.js
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (15)
test/foundry/BootstrapDepositNST.t.sol (1)
74-74: LGTM! Setting chain ID for consistent test environment.The addition of
vm.chainId(1)ensures that the capsule implementation can use default network constants consistently across all tests.test/foundry/unit/ExoCapsule.t.sol (2)
51-52: LGTM! Setting chain ID for deposit tests.The addition of
vm.chainId(1)ensures consistent test environment for deposit-related tests.
361-361: LGTM! Setting chain ID for withdrawal tests.The addition of
vm.chainId(1)ensures consistent test environment for withdrawal-related tests.test/foundry/unit/Bootstrap.t.sol (1)
88-89: LGTM! Setting chain ID for bootstrap tests.The addition of
vm.chainId(1)ensures consistent test environment for bootstrap-related tests. The empty line after the change improves readability.src/core/UTXOGateway.sol (1)
257-276: Validate Operator Address indelegateToFunctionWhile you check if the operator address is valid, adding a check to ensure the operator is registered may enhance security.
hardhat.config.js (3)
20-22: LGTM! Network configuration is properly set up for local testing.The network configuration for
exocore_localnetis correctly configured with a local URL and appropriate chain ID.
8-8: Verify compatibility with the new Solidity version.The upgrade to Solidity 0.8.22 is significant. Ensure all dependencies and smart contracts are compatible with this version.
Run the following script to check for potential compatibility issues:
✅ Verification successful
Solidity 0.8.22 compatibility verified ✅
All contracts in the codebase use pragma versions that are compatible with Solidity 0.8.22. The upgrade is safe to proceed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential compatibility issues with Solidity 0.8.22 # Test: Search for any explicit version requirements in dependencies rg -g "package.json" -A 5 '"solidity":' # Test: Search for any pragma statements that might conflict rg -g "*.sol" "pragma solidity"Length of output: 6454
24-30: Ensure environment variables are properly documented.The configuration uses multiple environment variables for test account private keys. These should be documented in the project's README or .env.example file.
Run the following script to check if these environment variables are documented:
package.json (2)
4-5: LGTM! Project metadata is well-defined.The description is comprehensive and the main entry point correctly points to
hardhat.config.js.
11-14: LGTM! Scripts are properly configured for testing and deployment.The scripts section includes necessary commands for testing and deploying the UTXOGateway contract.
test/mocks/AssetsMock.sol (2)
12-13: LGTM! Constants are properly defined for BTC integration.The addition of
VIRTUAL_STAKED_BTC_ADDRESSandclientBtcChainIdconstants supports BTC staking functionality.
77-84: LGTM! Withdrawal logic properly handles different token types.The withdrawal function correctly validates ETH and BTC addresses using clear boolean flags.
src/interfaces/precompiles/IAssets.sol (2)
10-24: LGTM! TokenInfo struct is well-defined and documented.The struct includes all necessary fields with clear documentation for each field.
26-45: LGTM! StakerBalance struct provides comprehensive staking information.The struct effectively captures all aspects of a staker's balance with clear documentation.
src/libraries/Errors.sol (1)
317-401: LGTM! UTXOGateway errors are comprehensive and well-documented.The error definitions follow best practices:
- Clear naming convention
- Descriptive comments
- Appropriate use of parameters where needed
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
src/interfaces/precompiles/IAssets.sol (1)
190-198: Add missing parameter documentation.The
@param stakerAddressdescription is missing in the function documentation.test/mocks/AssetsMock.sol (2)
77-84: Clean withdrawal logic with proper asset type validation!Consider extracting the asset type checks into helper functions for better reusability:
+ function isEthAsset(bytes32 assetAddressBytes32) internal pure returns (bool) { + return assetAddressBytes32 == bytes32(bytes20(VIRTUAL_STAKED_ETH_ADDRESS)); + } + + function isBtcAsset(bytes32 assetAddressBytes32) internal pure returns (bool) { + return assetAddressBytes32 == bytes32(bytes20(VIRTUAL_STAKED_BTC_ADDRESS)); + }
219-236: Document mock behavior of balance fields.The implementation returns identical values for balance fields and zeros for delegation-related fields. Add a comment explaining this simplified mock behavior.
function getStakerBalanceByToken(uint32 clientChainId, bytes calldata stakerAddress, bytes calldata token) external view returns (bool, StakerBalance memory) { + // Mock implementation: Returns same value for balance fields and zeros for delegation + // In production, these values would be different based on actual staking state return ( true, StakerBalance(src/storage/UTXOGatewayStorage.sol (2)
475-491: AdjustisValidOperatorAddressto accommodate variable-length Exocore addressesThe
isValidOperatorAddressfunction currently checks if the provided address string has a length of exactly 42 characters. However, Bech32 addresses (used by Exocore) can vary in length due to different encoding schemes and address types. Relying solely on a fixed length may lead to valid addresses being incorrectly rejected.Consider modifying the length check to allow for a range of valid lengths and ensure only the prefix is strictly validated:
function isValidOperatorAddress(string calldata addressToValidate) public pure returns (bool) { bytes memory stringBytes = bytes(addressToValidate); - if (stringBytes.length != 42) { + if (stringBytes.length < 42 || stringBytes.length > 62) { return false; } for (uint256 i = 0; i < EXO_ADDRESS_PREFIX.length; ++i) { if (stringBytes[i] != EXO_ADDRESS_PREFIX[i]) { return false; } } return true; }
85-90: Optimize storage inTransactionstruct by avoiding nested mappingsThe
Transactionstruct contains a nested mappingwitnessTime:mapping(address => uint256) witnessTime;Storing mappings within structs can lead to higher gas costs due to increased complexity in storage access.
Consider refactoring the storage of
witnessTimeto a separate mapping in the contract scope to reduce gas consumption:struct Transaction { TxStatus status; uint256 proofCount; uint256 expiryTime; - mapping(address => uint256) witnessTime; StakeMsg stakeMsg; } +mapping(bytes32 => mapping(address => uint256)) public witnessTimes;Update the access to
witnessTimeaccordingly in the contract functions.src/core/UTXOGateway.sol (1)
257-276: Refactor duplication indelegateToandundelegateFromfunctionsThe
delegateToandundelegateFromfunctions have similar logic for input validation and processing. Refactoring common code into an internal function can improve readability and maintainability.Extract shared code into an internal function:
+function _validateAndProcessDelegation( + Token token, + string calldata operator, + uint256 amount +) internal isValidAmount(amount) isRegistered(token, msg.sender) { + if (!isValidOperatorAddress(operator)) { + revert Errors.InvalidOperator(); + } +} function delegateTo(Token token, string calldata operator, uint256 amount) external nonReentrant whenNotPaused - isValidAmount(amount) - isRegistered(token, msg.sender) +{ + _validateAndProcessDelegation(token, operator, amount); // Existing logic... } function undelegateFrom(Token token, string calldata operator, uint256 amount) external nonReentrant whenNotPaused - isValidAmount(amount) - isRegistered(token, msg.sender) +{ + _validateAndProcessDelegation(token, operator, amount); // Existing logic... }package.json (1)
41-41: Move hardhat-gas-reporter to devDependencies.The
hardhat-gas-reporterpackage is a development tool and should be indevDependenciesinstead ofdependencies.- "dependencies": { - "hardhat-gas-reporter": "^1.0.9" - } + "devDependencies": { + "hardhat-gas-reporter": "^1.0.9" + }test/hardhat/integration/btc-stake-e2e.test.js (1)
25-54: Add block height validation in waitForBitcoinConfirmation.While the function correctly checks confirmations, it should validate that the block height increases between checks to detect potential chain reorganizations.
async function waitForBitcoinConfirmation(txid, confirmations = 1) { console.log(`Waiting for ${confirmations} confirmation(s) for tx: ${txid}`); + let lastBlockHeight = 0; while (true) { try { const response = await axios.get(`${BITCOIN_ESPLORA_API_URL}/api/tx/${txid}`); const tx = response.data; if (tx.status && tx.status.confirmed) { // Get current block height const blockInfoResponse = await axios.get(`${BITCOIN_ESPLORA_API_URL}/api/blocks/tip/height`); const currentHeight = parseInt(blockInfoResponse.data); + if (currentHeight < lastBlockHeight) { + console.log('Chain reorganization detected, resetting confirmation count'); + lastBlockHeight = currentHeight; + continue; + } + lastBlockHeight = currentHeight; const txHeight = tx.status.block_height; const currentConfirmations = currentHeight - txHeight + 1;🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (67)
.gitignore(1 hunks)hardhat.config.js(2 hunks)package.json(1 hunks)remappings.txt(0 hunks)script/10_DeployExocoreGatewayOnly.s.sol(3 hunks)script/11_SetPeers.s.sol(2 hunks)script/12_RedeployClientChainGateway.s.sol(3 hunks)script/13_DepositValidator.s.sol(2 hunks)script/14_CorrectBootstrapErrors.s.sol(4 hunks)script/15_DeploySafeMulstisigWallet.s.sol(1 hunks)script/16_UpgradeExoCapsule.s.sol(1 hunks)script/17_WithdrawalValidator.s.sol(4 hunks)script/18_SimulateReceive.s.sol(1 hunks)script/19_DeployFaucet.s.sol(1 hunks)script/1_Prerequisites.s.sol(2 hunks)script/2_DeployBoth.s.sol(3 hunks)script/3_Setup.s.sol(2 hunks)script/4_Deposit.s.sol(2 hunks)script/5_Withdraw.s.sol(2 hunks)script/6_CreateExoCapsule.s.sol(3 hunks)script/7_DeployBootstrap.s.sol(3 hunks)script/8_RegisterValidatorsAndDelegate.s.sol(1 hunks)script/9_ExtendBootstrapTime.s.sol(1 hunks)script/BaseScript.sol(1 hunks)script/TestPrecompileErrorFixed.s.sol(0 hunks)script/TestPrecompileErrorFixed_Deploy.s.sol(0 hunks)script/TokenTransfer.s.sol(0 hunks)script/deployBeaconOracle.s.sol(1 hunks)script/deployments/deployedContracts.json(1 hunks)script/deployments/utxo-gateway-exocore_localnet.json(1 hunks)script/hardhat/deploy-and-setup-utxogateway.script.js(1 hunks)slither.config.json(1 hunks)src/core/BaseRestakingController.sol(1 hunks)src/core/ClientChainGateway.sol(1 hunks)src/core/ExocoreGateway.sol(1 hunks)src/core/RewardVault.sol(1 hunks)src/core/UTXOGateway.sol(1 hunks)src/interfaces/IClientChainGateway.sol(1 hunks)src/interfaces/IExocoreGateway.sol(1 hunks)src/interfaces/precompiles/IAssets.sol(6 hunks)src/libraries/Errors.sol(1 hunks)src/libraries/ExocoreBytes.sol(1 hunks)src/libraries/SignatureVerifier.sol(1 hunks)src/lzApp/OAppCoreUpgradeable.sol(1 hunks)src/lzApp/OAppReceiverUpgradeable.sol(1 hunks)src/lzApp/OAppSenderUpgradeable.sol(1 hunks)src/storage/UTXOGatewayStorage.sol(1 hunks)test/foundry/Delegation.t.sol(1 hunks)test/foundry/DepositThenDelegateTo.t.sol(1 hunks)test/foundry/DepositWithdrawPrinciple.t.sol(1 hunks)test/foundry/ExocoreDeployer.t.sol(1 hunks)test/foundry/Governance.t.sol(1 hunks)test/foundry/TvlLimits.t.sol(1 hunks)test/foundry/unit/ClientChainGateway.t.sol(1 hunks)test/foundry/unit/ExoCapsule.t.sol(0 hunks)test/foundry/unit/ExocoreGateway.t.sol(2 hunks)test/foundry/unit/UTXOGateway.t.sol(1 hunks)test/hardhat/client-chain/deploy_fixture.js(0 hunks)test/hardhat/client-chain/gateway.test.js(0 hunks)test/hardhat/exocore/deploy_fixture.js(0 hunks)test/hardhat/exocore/gateway.test.js(0 hunks)test/hardhat/integration/btc-stake-e2e.test.js(1 hunks)test/hardhat/integration/btc-stake.test.js(1 hunks)test/mocks/AssetsMock.sol(6 hunks)test/mocks/DelegationMock.sol(4 hunks)test/mocks/ExocoreGatewayMock.sol(1 hunks)test/mocks/NonShortCircuitLzEndpointMock.sol(0 hunks)
💤 Files with no reviewable changes (10)
- test/hardhat/exocore/gateway.test.js
- test/foundry/unit/ExoCapsule.t.sol
- test/hardhat/client-chain/gateway.test.js
- script/TokenTransfer.s.sol
- script/TestPrecompileErrorFixed.s.sol
- test/mocks/NonShortCircuitLzEndpointMock.sol
- remappings.txt
- test/hardhat/client-chain/deploy_fixture.js
- script/TestPrecompileErrorFixed_Deploy.s.sol
- test/hardhat/exocore/deploy_fixture.js
🚧 Files skipped from review as they are similar to previous changes (48)
- script/9_ExtendBootstrapTime.s.sol
- script/15_DeploySafeMulstisigWallet.s.sol
- .gitignore
- script/2_DeployBoth.s.sol
- script/6_CreateExoCapsule.s.sol
- script/8_RegisterValidatorsAndDelegate.s.sol
- src/lzApp/OAppSenderUpgradeable.sol
- script/16_UpgradeExoCapsule.s.sol
- src/lzApp/OAppReceiverUpgradeable.sol
- src/lzApp/OAppCoreUpgradeable.sol
- script/3_Setup.s.sol
- script/19_DeployFaucet.s.sol
- script/11_SetPeers.s.sol
- script/BaseScript.sol
- src/core/BaseRestakingController.sol
- script/18_SimulateReceive.s.sol
- script/17_WithdrawalValidator.s.sol
- script/7_DeployBootstrap.s.sol
- src/core/RewardVault.sol
- test/foundry/DepositWithdrawPrinciple.t.sol
- test/foundry/Delegation.t.sol
- test/foundry/Governance.t.sol
- script/5_Withdraw.s.sol
- script/10_DeployExocoreGatewayOnly.s.sol
- slither.config.json
- src/libraries/ExocoreBytes.sol
- test/foundry/ExocoreDeployer.t.sol
- test/foundry/TvlLimits.t.sol
- src/core/ClientChainGateway.sol
- script/4_Deposit.s.sol
- script/14_CorrectBootstrapErrors.s.sol
- test/foundry/DepositThenDelegateTo.t.sol
- script/deployments/utxo-gateway-exocore_localnet.json
- src/core/ExocoreGateway.sol
- test/mocks/ExocoreGatewayMock.sol
- script/deployments/deployedContracts.json
- src/interfaces/IClientChainGateway.sol
- script/deployBeaconOracle.s.sol
- script/12_RedeployClientChainGateway.s.sol
- test/foundry/unit/ExocoreGateway.t.sol
- script/1_Prerequisites.s.sol
- src/interfaces/IExocoreGateway.sol
- script/13_DepositValidator.s.sol
- test/foundry/unit/ClientChainGateway.t.sol
- src/libraries/SignatureVerifier.sol
- test/mocks/DelegationMock.sol
- test/hardhat/integration/btc-stake.test.js
- test/foundry/unit/UTXOGateway.t.sol
🧰 Additional context used
📓 Learnings (4)
test/mocks/AssetsMock.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#23
File: test/mocks/AssetsMock.sol:9-28
Timestamp: 2024-11-12T10:03:05.536Z
Learning: The `AssetsMock` contract is used for simulating a production precompiled contract, and its complexity in data structures is intentional to mimic production behavior.
src/storage/UTXOGatewayStorage.sol (2)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/storage/UTXOGatewayStorage.sol:457-462
Timestamp: 2025-01-20T02:28:24.440Z
Learning: In the UTXOGateway contract, Token.NONE cannot be registered in outboundRegistry because registration requires a valid ClientChainID and goes through _registerAddress function which has multiple validations. The clientChainId parameter comes from validated sources (stake messages, etc.), making additional Token.NONE checks in the isRegistered modifier redundant.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/storage/UTXOGatewayStorage.sol:457-462
Timestamp: 2025-01-20T02:28:24.440Z
Learning: In the UTXOGateway contract, Token.NONE cannot be registered in outboundRegistry due to validations during the registration process, making additional Token.NONE checks in the isRegistered modifier redundant.
src/core/UTXOGateway.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/core/UTXOGateway.sol:407-424
Timestamp: 2025-01-08T03:15:08.317Z
Learning: The `onlyAuthorizedWitness` modifier checking if `msg.sender` is an authorized witness provides sufficient access control for functions that should only be called by witnesses in the UTXOGateway contract.
test/hardhat/integration/btc-stake-e2e.test.js (2)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/hardhat/integration/btc-stake-e2e.test.js:0-0
Timestamp: 2025-01-20T02:24:57.006Z
Learning: In the Exocore project's test suite, prefer using interval-based contract state querying over event listeners for monitoring state changes, as it provides better control and reliability.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/hardhat/integration/btc-stake-e2e.test.js:24-36
Timestamp: 2025-01-08T03:12:49.494Z
Learning: In Bitcoin transaction tests, prefer using test-level timeouts over function-level timeouts to handle long-running operations, as it provides a single point of control and avoids redundant timeout handling.
🪛 Biome (1.9.4)
test/hardhat/integration/btc-stake-e2e.test.js
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (11)
src/interfaces/precompiles/IAssets.sol (3)
10-24: Well-structured token information model!The
TokenInfostruct provides a comprehensive and well-documented model for token metadata.
174-178: Well-designed query function with proper error handling!The function follows the pattern of returning a success flag along with the result.
180-188: Clean token info query design!The function follows good practices with proper error handling and documentation.
test/mocks/AssetsMock.sol (1)
12-13: Well-structured BTC support implementation!The code properly handles BTC-specific validation with clear separation from ETH logic.
Also applies to: 37-48
src/core/UTXOGateway.sol (1)
846-863: Ensure proper fee calculation and handle potential rounding errorsThe fee calculation in
_processStakeMsguses integer division, which may lead to rounding errors for small amounts.Verify if the integer division in fee calculation accurately reflects the intended fee structure, especially for small transaction amounts. Consider using a library or safe math practices to handle fixed-point arithmetic if necessary.
✅ Verification successful
Fee calculation implementation is safe and follows best practices
The fee calculation uses a standard basis points approach (1 bp = 0.01%) with proper constraints:
- Maximum fee rate is capped at 10% (1000 basis points)
- Integer division rounding down is the expected behavior for fee calculations
- The basis points denominator (10000) provides sufficient granularity for typical fee rates
The implementation is safe and doesn't require additional safeguards against rounding errors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential rounding issues in fee calculation # Locate all instances of fee calculations rg "uint256 fee = .*" -A2 # Review the calculations for correct implementationLength of output: 221
Script:
#!/bin/bash # Find BASIS_POINTS constant and bridgeFeeRate related code rg "BASIS_POINTS\s*=|bridgeFeeRate" -B2 -A2 # Look for minimum amount checks and related events/errors ast-grep --pattern 'require($_amount $_, $_)' ast-grep --pattern 'if ($_amount $_, $_)' rg "event.*Fee|error.*Fee"Length of output: 4174
hardhat.config.js (2)
24-30:⚠️ Potential issueValidate environment variables for accounts are properly configured
The accounts array relies on several environment variables. Missing or improperly set variables could lead to deployment failures.
Ensure that all required environment variables are set and valid:
LOCAL_EXOCORE_FUNDED_ACCOUNT_PRIVATE_KEYTEST_ACCOUNT_ONE_PRIVATE_KEYTEST_ACCOUNT_TWO_PRIVATE_KEYTEST_ACCOUNT_THREE_PRIVATE_KEYTEST_ACCOUNT_FOUR_PRIVATE_KEYTEST_ACCOUNT_FIVE_PRIVATE_KEYTEST_ACCOUNT_SIX_PRIVATE_KEYIf any variables are missing, the network configuration will fail during deployment. Consider adding checks or fallback mechanisms.
8-8: Confirm compatibility with Solidity version0.8.22Updating Solidity compiler to version
0.8.22may introduce changes that affect contract behavior or compatibility with dependencies.Ensure that all contracts and dependencies are compatible with Solidity
0.8.22. Review the change logs for any breaking changes between0.8.19and0.8.22.✅ Verification successful
Solidity version
0.8.22is compatible with the codebaseThe codebase primarily uses
^0.8.19which is compatible with0.8.22. All other version constraints (^0.8.0,^0.8.13,>=0.8.17) are also compatible with this upgrade. No version-specific features or breaking changes between0.8.19and0.8.22would affect the contracts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify dependencies compatibility with Solidity 0.8.22 # List all Solidity files specifying pragma version rg --type solidity "pragma solidity" -l # Check for any version-specific code that may be affectedLength of output: 74
Script:
#!/bin/bash # Find all Solidity files and their pragma directives echo "=== Solidity Files and Their Pragma Directives ===" fd -e sol --exec grep -l "pragma solidity" {} \; || true fd -e sol --exec cat {} \; || true # Check for package.json files that might contain Solidity-related dependencies echo -e "\n=== Package Dependencies ===" fd package.json --exec cat {} \; || trueLength of output: 72342
Script:
#!/bin/bash # Extract pragma directives from all Solidity files echo "=== Pragma Directives ===" fd -e sol --exec grep -h "pragma solidity" {} \; | sort -u # Check for any version-specific imports or dependencies echo -e "\n=== Version-specific Dependencies ===" rg -i "0\.8\." --type-add 'sol:*.sol' -g '*.sol' -A 1 -B 1 || trueLength of output: 15827
script/hardhat/deploy-and-setup-utxogateway.script.js (1)
24-26: LGTM! Proper balance verification for initial accounts.The script correctly ensures all accounts have sufficient balance before proceeding with deployment.
src/libraries/Errors.sol (1)
317-401: LGTM! Well-organized error definitions for UTXOGateway.The error definitions are comprehensive and well-documented, covering all necessary scenarios for the UTXOGateway functionality including:
- Witness management
- Transaction processing
- Request handling
- Authorization checks
- State validation
test/hardhat/integration/btc-stake-e2e.test.js (2)
314-392: LGTM! Robust test implementation.The test case follows best practices:
- Uses interval-based state checking instead of event listeners
- Implements proper timeout handling at the test level
- Validates both initial and final balances
- Includes comprehensive error handling
221-224: Verify OP_RETURN data format.The EVM address is correctly added to OP_RETURN, but we should verify that the UTXOGateway contract expects this exact format.
✅ Verification successful
OP_RETURN format is correct and compatible with UTXOGateway
The UTXOGateway contract expects raw EVM addresses without any special encoding, which matches exactly how the test constructs the OP_RETURN output (raw hex format). The contract's address handling and mapping functions confirm this is the correct format.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check how the UTXOGateway contract parses OP_RETURN data ast-grep --pattern 'function $_(bytes memory $data) $$ { $$$ address $_ = $$$; $$$ }'Length of output: 93
Script:
#!/bin/bash # Search for OP_RETURN related code rg -i "op_return" -A 5 # Look for Bitcoin transaction processing ast-grep --pattern 'function process$_($_) { $$$ }' # Search for potential address extraction rg -i "extract.*address" -A 5Length of output: 747
Script:
#!/bin/bash # Find contract files fd -e sol # Search for Bitcoin/BTC related contracts rg -l -i "bitcoin|btc" --type sol # Search for staking and gateway contracts rg -l -i "stake|gateway|bridge" --type solLength of output: 3817
Script:
#!/bin/bash # Check UTXOGateway.sol content cat src/core/UTXOGateway.sol # Search for specific patterns in UTXOGateway rg -p "function" src/core/UTXOGateway.sol # Look for byte manipulation or parsing rg "bytes" src/core/UTXOGateway.solLength of output: 41129
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (17)
test/hardhat/integration/btc-stake-e2e.test.js (7)
21-23: Improve error message to include all checked variables.The error message doesn't mention
BITCOIN_STAKER_PRIVATE_KEYwhich is also being checked.- throw new Error('BITCOIN_ESPLORA_API_URL or TEST_ACCOUNT_THREE_PRIVATE_KEY or BITCOIN_VAULT_ADDRESS is not set'); + throw new Error('One or more required environment variables are not set: BITCOIN_ESPLORA_API_URL, TEST_ACCOUNT_THREE_PRIVATE_KEY, BITCOIN_VAULT_ADDRESS, BITCOIN_STAKER_PRIVATE_KEY');
33-34: Use optional chaining for safer property access.Using optional chaining would make the code more robust when handling API responses.
- if (tx.status && tx.status.confirmed) { + if (tx.status?.confirmed) {🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
49-51: Improve error handling specificity.The current error handling logs all errors with the same message. Consider handling specific error cases differently.
- console.log('Error checking transaction status:', error.message); + if (error.response?.status === 404) { + console.log('Transaction not yet indexed...'); + } else if (error.code === 'ECONNREFUSED') { + console.log('Bitcoin node connection failed:', error.message); + } else { + console.log('Error checking transaction status:', error.message); + }
115-122: Extract magic numbers as named constants.The code uses several magic numbers that would be more maintainable as named constants.
+const SATS_PER_BTC = 100000000; +const MIN_CHANGE_VALUE = 546; // dust threshold +const DEFAULT_FEE = 1000; // satoshis - const fee = 1000; + const fee = DEFAULT_FEE; - const change = totalInput - (amount * 100000000) - fee; + const change = totalInput - (amount * SATS_PER_BTC) - fee; - if (change > 546) { + if (change > MIN_CHANGE_VALUE) {
181-181: Make requiredBalance configurable.The required balance is hardcoded to 0.01 BTC. Consider making this configurable or at least a named constant.
- const requiredBalance = 0.01; // set required balance to 0.01 BTC + const REQUIRED_BALANCE = 0.01; // BTC + const requiredBalance = process.env.REQUIRED_BALANCE || REQUIRED_BALANCE;
392-392: Add comment explaining the long timeout value.The test has a 5-minute timeout. Add a comment explaining why this duration is necessary.
- }).timeout(300000); + }).timeout(300000); // 5 minutes: allows time for Bitcoin confirmation and bridge processing
360-374: Extract balance verification logic to a helper function.The balance verification logic is complex and could be reused. Consider extracting it to a helper function.
+ async function verifyBalance(initialBalance, finalBalance, expectedAmount) { + expect(finalBalance[0]).to.equal(CLIENT_CHAIN.BTC); + expect(ethers.hexlify(finalBalance[1])).to.equal(ethers.hexlify(staker.address)); + expect(ethers.hexlify(finalBalance[2])).to.equal(ethers.hexlify(BTC_ID)); + expect(finalBalance[3] - (initialBalance ? initialBalance[3] : 0)).to.equal(expectedAmount); + expect(finalBalance[7] - (initialBalance ? initialBalance[7] : 0)).to.equal(expectedAmount); + } // Verify final balance const [finalSuccess, finalBalance] = await assetsPrecompile.getStakerBalanceByToken( CLIENT_CHAIN.BTC, ethers.getBytes(staker.address), BTC_ID ); expect(finalSuccess).to.be.true; const expectedIncrease = ethers.parseUnits('0.01', 8); - - expect(finalBalance[0]).to.equal(CLIENT_CHAIN.BTC); - expect(ethers.hexlify(finalBalance[1])).to.equal(ethers.hexlify(staker.address)); - expect(ethers.hexlify(finalBalance[2])).to.equal(ethers.hexlify(BTC_ID)); - expect(finalBalance[3] - (initialBalance ? initialBalance[3] : 0)).to.equal(expectedIncrease); - expect(finalBalance[7] - (initialBalance ? initialBalance[7] : 0)).to.equal(expectedIncrease); + await verifyBalance(initialBalance, finalBalance, expectedIncrease);src/core/UTXOGateway.sol (4)
163-164: Use custom errors instead ofrequirewith string messagesIn the
updateBridgeFeeRatefunction, consider replacing therequirestatement with a custom error from theErrorslibrary to align with the rest of the contract's error handling and to save gas.Apply this diff to use a custom error:
function updateBridgeFeeRate(uint256 bridgeFeeRate_) external onlyOwner whenNotPaused { - require(bridgeFeeRate_ <= MAX_BRIDGE_FEE_RATE, "Fee cannot exceed max bridge fee rate"); + if (bridgeFeeRate_ > MAX_BRIDGE_FEE_RATE) { + revert Errors.ExceededMaxBridgeFeeRate(); + } bridgeFeeRate = bridgeFeeRate_; emit BridgeFeeRateUpdated(bridgeFeeRate_); }
820-823: Use custom errors for input validation in_registerAddressIn the
_registerAddressfunction, replacerequirestatements with custom errors from theErrorslibrary for consistency and gas optimization.Apply this diff:
function _registerAddress(ClientChainID clientChainId, bytes memory depositor, address exocoreAddress) internal { - require(depositor.length > 0 && exocoreAddress != address(0), "Invalid address"); - require(inboundRegistry[clientChainId][depositor] == address(0), "Depositor address already registered"); - require(outboundRegistry[clientChainId][exocoreAddress].length == 0, "Exocore address already registered"); + if (depositor.length == 0) { + revert Errors.InvalidDepositorAddress(); + } + if (exocoreAddress == address(0)) { + revert Errors.ZeroAddress(); + } + if (inboundRegistry[clientChainId][depositor] != address(0)) { + revert Errors.DepositorAlreadyRegistered(); + } + if (outboundRegistry[clientChainId][exocoreAddress].length != 0) { + revert Errors.ExocoreAddressAlreadyRegistered(); + }
574-575: Provide specific error for invalid witness addressIn the
_addWitnessfunction, when checking if_witnessis the zero address, consider using a more specific custom error to make debugging easier.Apply this diff:
if (_witness == address(0)) { - revert Errors.ZeroAddress(); + revert Errors.InvalidWitnessAddress(); }
692-693: Provide detailed error messages for invalid stake messagesIn the
_verifyStakeMsgFieldsfunction, consider adding specific error checks for each invalid field to help identify issues quickly during debugging.Apply this diff:
if (uint8(_msg.clientChainId) == 0) { revert Errors.InvalidClientChainId(); } if (_msg.nonce == 0) { revert Errors.InvalidNonce(); } if (uint256(_msg.clientTxId) == 0) { revert Errors.InvalidClientTxId(); } if (_msg.clientAddress.length == 0) { revert Errors.InvalidClientAddress(); } if (_msg.amount == 0) { revert Errors.InvalidAmount(); } if (bytes(_msg.operator).length > 0 && !isValidOperatorAddress(_msg.operator)) { revert Errors.InvalidOperator(); }test/mocks/AssetsMock.sol (1)
177-184: Input validation forupdateAuthorizedGatewaysfunctionWhile duplicate gateway addresses may not cause issues, consider adding input validation to check for zero addresses to prevent unauthorized access.
Apply this diff:
function updateAuthorizedGateways(address[] calldata gateways) external returns (bool) { if (gateways.length == 0) { return false; } for (uint256 i = 0; i < gateways.length; i++) { + if (gateways[i] == address(0)) { + return false; + } authorizedGateways[gateways[i]] = true; } return true; }src/libraries/Errors.sol (4)
321-335: Consider adding error cases for additional edge cases.While the core UTXO operation errors are well-defined, consider adding the following error cases for better edge case handling:
- Amount validation errors (e.g.,
InvalidStakeAmount)- Script validation errors (e.g.,
InvalidScriptFormat)- Transaction size/weight limits (e.g.,
TxSizeExceedsLimit)
336-344: Enhance error messages with additional context.Consider adding more context to witness-related errors:
- error WitnessAlreadyAuthorized(address witness); + error WitnessAlreadyAuthorized(address witness, uint256 witnessCount); - error WitnessNotAuthorized(address witness); + error WitnessNotAuthorized(address witness, address[] currentWitnesses); - error CannotRemoveLastWitness(); + error CannotRemoveLastWitness(address lastWitness);
345-401: Consider reorganizing request-related errors for better readability.The request handling errors would be more maintainable if grouped by functionality:
- Request State Errors:
- RequestNotFound
- RequestAlreadyExists
- RequestAlreadyProcessed
- Consensus Errors:
- ConsensusNotRequired
- ConsensusRequired
- UnauthorizedWitness
- UnauthorizedProposer
- Nonce Management Errors:
- InvalidRequestNonce
- RequestNonceOutOfBounds
- Transaction Errors:
- InvalidPegoutTxId
- TxIdNotFound
317-401: Maintain consistent error documentation style.The UTXOGateway errors section follows good practices but could benefit from more detailed documentation:
- Add
@paramdescriptions for error parameters (like in other sections, e.g.,PrecompileCallFailed)- Include example scenarios in comments where the errors might occur
- Consider adding severity levels to errors (e.g., critical vs. recoverable)
script/hardhat/deploy-and-setup-utxogateway.script.js (1)
19-27: Add gas price management for fund distribution.The initial fund distribution should include gas price estimation and management to handle network congestion.
for (const account of initialAccounts) { + const gasPrice = await ethers.provider.getGasPrice(); const tx = await faucet.sendTransaction({ to: account.address, value: ethers.parseEther("1"), + gasPrice: gasPrice, }); - // wait until the transaction is mined but should not exceed 10 seconds - await tx.wait(); + // wait with timeout + await Promise.race([ + tx.wait(), + new Promise((_, reject) => + setTimeout(() => reject(new Error('Transaction timeout')), 10000) + ) + ]); assert(await ethers.provider.getBalance(account.address) > 0, "no enough balance for account") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (67)
.gitignore(1 hunks)hardhat.config.js(2 hunks)package.json(1 hunks)remappings.txt(0 hunks)script/10_DeployExocoreGatewayOnly.s.sol(3 hunks)script/11_SetPeers.s.sol(2 hunks)script/12_RedeployClientChainGateway.s.sol(3 hunks)script/13_DepositValidator.s.sol(2 hunks)script/14_CorrectBootstrapErrors.s.sol(4 hunks)script/15_DeploySafeMulstisigWallet.s.sol(1 hunks)script/16_UpgradeExoCapsule.s.sol(1 hunks)script/17_WithdrawalValidator.s.sol(4 hunks)script/18_SimulateReceive.s.sol(1 hunks)script/19_DeployFaucet.s.sol(1 hunks)script/1_Prerequisites.s.sol(2 hunks)script/2_DeployBoth.s.sol(3 hunks)script/3_Setup.s.sol(2 hunks)script/4_Deposit.s.sol(2 hunks)script/5_Withdraw.s.sol(2 hunks)script/6_CreateExoCapsule.s.sol(3 hunks)script/7_DeployBootstrap.s.sol(3 hunks)script/8_RegisterValidatorsAndDelegate.s.sol(1 hunks)script/9_ExtendBootstrapTime.s.sol(1 hunks)script/BaseScript.sol(1 hunks)script/TestPrecompileErrorFixed.s.sol(0 hunks)script/TestPrecompileErrorFixed_Deploy.s.sol(0 hunks)script/TokenTransfer.s.sol(0 hunks)script/deployBeaconOracle.s.sol(1 hunks)script/deployments/deployedContracts.json(1 hunks)script/deployments/utxo-gateway-exocore_localnet.json(1 hunks)script/hardhat/deploy-and-setup-utxogateway.script.js(1 hunks)slither.config.json(1 hunks)src/core/BaseRestakingController.sol(1 hunks)src/core/ClientChainGateway.sol(1 hunks)src/core/ExocoreGateway.sol(1 hunks)src/core/RewardVault.sol(1 hunks)src/core/UTXOGateway.sol(1 hunks)src/interfaces/IClientChainGateway.sol(1 hunks)src/interfaces/IExocoreGateway.sol(1 hunks)src/interfaces/precompiles/IAssets.sol(6 hunks)src/libraries/Errors.sol(1 hunks)src/libraries/ExocoreBytes.sol(1 hunks)src/libraries/SignatureVerifier.sol(1 hunks)src/lzApp/OAppCoreUpgradeable.sol(1 hunks)src/lzApp/OAppReceiverUpgradeable.sol(1 hunks)src/lzApp/OAppSenderUpgradeable.sol(1 hunks)src/storage/UTXOGatewayStorage.sol(1 hunks)test/foundry/Delegation.t.sol(1 hunks)test/foundry/DepositThenDelegateTo.t.sol(1 hunks)test/foundry/DepositWithdrawPrinciple.t.sol(1 hunks)test/foundry/ExocoreDeployer.t.sol(1 hunks)test/foundry/Governance.t.sol(1 hunks)test/foundry/TvlLimits.t.sol(1 hunks)test/foundry/unit/ClientChainGateway.t.sol(1 hunks)test/foundry/unit/ExoCapsule.t.sol(0 hunks)test/foundry/unit/ExocoreGateway.t.sol(2 hunks)test/foundry/unit/UTXOGateway.t.sol(1 hunks)test/hardhat/client-chain/deploy_fixture.js(0 hunks)test/hardhat/client-chain/gateway.test.js(0 hunks)test/hardhat/exocore/deploy_fixture.js(0 hunks)test/hardhat/exocore/gateway.test.js(0 hunks)test/hardhat/integration/btc-stake-e2e.test.js(1 hunks)test/hardhat/integration/btc-stake.test.js(1 hunks)test/mocks/AssetsMock.sol(6 hunks)test/mocks/DelegationMock.sol(4 hunks)test/mocks/ExocoreGatewayMock.sol(1 hunks)test/mocks/NonShortCircuitLzEndpointMock.sol(0 hunks)
💤 Files with no reviewable changes (10)
- test/hardhat/client-chain/gateway.test.js
- test/hardhat/exocore/gateway.test.js
- remappings.txt
- test/foundry/unit/ExoCapsule.t.sol
- test/hardhat/client-chain/deploy_fixture.js
- script/TestPrecompileErrorFixed.s.sol
- script/TokenTransfer.s.sol
- script/TestPrecompileErrorFixed_Deploy.s.sol
- test/mocks/NonShortCircuitLzEndpointMock.sol
- test/hardhat/exocore/deploy_fixture.js
🚧 Files skipped from review as they are similar to previous changes (49)
- .gitignore
- script/9_ExtendBootstrapTime.s.sol
- script/19_DeployFaucet.s.sol
- src/core/RewardVault.sol
- script/8_RegisterValidatorsAndDelegate.s.sol
- script/6_CreateExoCapsule.s.sol
- script/15_DeploySafeMulstisigWallet.s.sol
- src/libraries/ExocoreBytes.sol
- script/deployBeaconOracle.s.sol
- script/18_SimulateReceive.s.sol
- script/5_Withdraw.s.sol
- script/2_DeployBoth.s.sol
- script/11_SetPeers.s.sol
- test/foundry/DepositWithdrawPrinciple.t.sol
- script/7_DeployBootstrap.s.sol
- test/foundry/ExocoreDeployer.t.sol
- src/core/ClientChainGateway.sol
- src/lzApp/OAppSenderUpgradeable.sol
- script/4_Deposit.s.sol
- src/core/BaseRestakingController.sol
- script/12_RedeployClientChainGateway.s.sol
- src/interfaces/IClientChainGateway.sol
- test/foundry/DepositThenDelegateTo.t.sol
- script/3_Setup.s.sol
- src/interfaces/IExocoreGateway.sol
- slither.config.json
- script/17_WithdrawalValidator.s.sol
- script/10_DeployExocoreGatewayOnly.s.sol
- script/14_CorrectBootstrapErrors.s.sol
- test/foundry/TvlLimits.t.sol
- script/16_UpgradeExoCapsule.s.sol
- src/lzApp/OAppCoreUpgradeable.sol
- src/lzApp/OAppReceiverUpgradeable.sol
- script/BaseScript.sol
- script/1_Prerequisites.s.sol
- test/hardhat/integration/btc-stake.test.js
- src/core/ExocoreGateway.sol
- test/mocks/ExocoreGatewayMock.sol
- test/foundry/Governance.t.sol
- test/foundry/unit/ExocoreGateway.t.sol
- test/foundry/Delegation.t.sol
- script/deployments/deployedContracts.json
- script/13_DepositValidator.s.sol
- script/deployments/utxo-gateway-exocore_localnet.json
- test/foundry/unit/ClientChainGateway.t.sol
- test/mocks/DelegationMock.sol
- src/libraries/SignatureVerifier.sol
- test/foundry/unit/UTXOGateway.t.sol
- src/storage/UTXOGatewayStorage.sol
🧰 Additional context used
📓 Learnings (4)
test/mocks/AssetsMock.sol (3)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#23
File: test/mocks/AssetsMock.sol:9-28
Timestamp: 2024-11-12T10:03:05.536Z
Learning: The `AssetsMock` contract is used for simulating a production precompiled contract, and its complexity in data structures is intentional to mimic production behavior.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/mocks/AssetsMock.sol:149-156
Timestamp: 2025-01-22T03:59:43.806Z
Learning: Mock implementations should strictly follow their interfaces without adding extra parameters or functionality beyond what's defined in the interface.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/mocks/AssetsMock.sol:177-186
Timestamp: 2025-01-22T03:57:41.625Z
Learning: In the `updateAuthorizedGateways` function of the `AssetsMock` contract, duplicate gateway addresses in the input array are acceptable as they don't cause any unexpected effects when setting the same authorization flag multiple times.
src/interfaces/precompiles/IAssets.sol (2)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/interfaces/precompiles/IAssets.sol:26-45
Timestamp: 2025-01-22T03:46:31.059Z
Learning: Interface files (.sol files containing only interface definitions) should not contain implementation details like validation logic - they should only define the contract structure and function signatures.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/interfaces/precompiles/IAssets.sol:158-162
Timestamp: 2025-01-22T02:55:03.094Z
Learning: Interface files (*.sol files prefixed with 'I') should only contain function declarations without implementation details or validation requirements. Implementation details belong in the concrete contract files.
src/core/UTXOGateway.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/core/UTXOGateway.sol:407-424
Timestamp: 2025-01-08T03:15:08.317Z
Learning: The `onlyAuthorizedWitness` modifier checking if `msg.sender` is an authorized witness provides sufficient access control for functions that should only be called by witnesses in the UTXOGateway contract.
test/hardhat/integration/btc-stake-e2e.test.js (2)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/hardhat/integration/btc-stake-e2e.test.js:0-0
Timestamp: 2025-01-20T02:24:57.006Z
Learning: In the Exocore project's test suite, prefer using interval-based contract state querying over event listeners for monitoring state changes, as it provides better control and reliability.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/hardhat/integration/btc-stake-e2e.test.js:24-36
Timestamp: 2025-01-08T03:12:49.494Z
Learning: In Bitcoin transaction tests, prefer using test-level timeouts over function-level timeouts to handle long-running operations, as it provides a single point of control and avoids redundant timeout handling.
🪛 Biome (1.9.4)
test/hardhat/integration/btc-stake-e2e.test.js
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (10)
test/mocks/AssetsMock.sol (1)
149-156: Confirm the handling of emptysymbolinTokenInfoIn the
registerTokenfunction, thesymbolfield inTokenInfois initialized as an empty string. Ensure that downstream contracts and functions can handle an emptysymbolappropriately.src/interfaces/precompiles/IAssets.sol (2)
26-45: Avoid implementation details in interface commentsThe comments for the
StakerBalancestruct include implementation-specific details. Interfaces should define the contract's API without implementation logic or validation requirements.
158-162: Exclude validation notes from interface definitionsIn the
updateAuthorizedGatewaysfunction, avoid including implementation or validation details in the interface. Such details belong in the contract's documentation or implementation files.src/libraries/Errors.sol (1)
317-320: LGTM! Section header follows established pattern.The UTXOGateway errors section is well-organized and consistent with the file's structure.
hardhat.config.js (2)
20-30: LGTM! Network configuration is well structured.The localnet configuration is properly set up with:
- Local network URL
- Appropriate chainId
- Environment variables for test accounts
8-8: Verify compatibility with Solidity 0.8.22.The upgrade to Solidity 0.8.22 is significant. Ensure all contracts are compatible with this version, particularly regarding breaking changes in this version.
✅ Verification successful
Solidity 0.8.22 upgrade is compatible with all contract files
The codebase uses version constraints (^0.8.x) that safely allow the upgrade to 0.8.22. All contracts, including interfaces and dependencies, have compatible pragma directives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all Solidity files and check their pragma versions echo "=== Solidity Files with their Pragma Versions ===" rg -B 1 -A 1 "pragma solidity"Length of output: 15410
package.json (2)
11-14: LGTM! Scripts are properly configured.The test and deployment scripts are well-defined and match the project's requirements.
19-36: Verify compatibility of Bitcoin-related dependencies.The Bitcoin-related packages (
bitcoinjs-lib,tiny-secp256k1,ecpair) are crucial for UTXO handling. Ensure these versions are compatible with each other.✅ Verification successful
Bitcoin-related dependencies are compatible ✅
The current versions of
[email protected],[email protected], and[email protected]are designed to work together without strict version coupling. No compatibility issues or conflicting dependencies were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any known compatibility issues between Bitcoin-related packages echo "=== Package Versions ===" npm view [email protected] peerDependencies npm view [email protected] peerDependencies npm view [email protected] peerDependenciesLength of output: 196
Script:
#!/bin/bash echo "=== bitcoinjs-lib Dependencies ===" npm view [email protected] dependencies echo -e "\n=== ecpair Dependencies ===" npm view [email protected] dependencies echo -e "\n=== tiny-secp256k1 Dependencies ===" npm view [email protected] dependenciesLength of output: 607
script/hardhat/deploy-and-setup-utxogateway.script.js (2)
127-139: LGTM! Deployment information is properly stored.The deployment information is comprehensively saved with all necessary details.
64-68: Verify initialization parameters against contract constraints.The initialization parameters (owner, witnesses, requiredProofs) should be verified against the contract's constraints before deployment.
✅ Verification successful
Initialization parameters are properly validated by the contract ✓
The contract's initialize function has comprehensive validation checks that ensure:
- Owner address is non-zero
- Witnesses array is non-empty
- Required proofs value is within bounds (1-10)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check UTXOGateway contract for initialization constraints echo "=== Initialization Constraints ===" rg -A 5 "initialize.*function" rg -A 5 "require.*witness"Length of output: 4691
Script:
#!/bin/bash # Search for initialize function and related validations in UTXOGateway echo "=== UTXOGateway Initialize Function ===" ast-grep --pattern 'function initialize($$$) { $$$ }' echo -e "\n=== Validation Checks ===" rg -A 5 "require.*initialize" rg -A 5 "require.*witness" rg -A 5 "require.*proof"Length of output: 18936
Script:
#!/bin/bash # Search for MIN_REQUIRED_PROOFS and MAX_REQUIRED_PROOFS constants echo "=== Required Proofs Constants ===" rg "MIN_REQUIRED_PROOFS|MAX_REQUIRED_PROOFS" -B 2 -A 2Length of output: 1274
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
.github/workflows/forge-ci.yml (1)
152-152: Add error handling for the file read operation.While the path reorganization to
script/deployments/improves the project structure, consider adding error handling for the file read operation.- data=$(cat script/deployments/deployedContracts.json) + if [ ! -f script/deployments/deployedContracts.json ]; then + echo "Error: deployedContracts.json not found at script/deployments/" + exit 1 + fi + data=$(cat script/deployments/deployedContracts.json) + if [ -z "$data" ]; then + echo "Error: deployedContracts.json is empty" + exit 1 + fitest/hardhat/integration/btc-stake-e2e.test.js (6)
12-14: Consider extracting hardcoded addresses into a configuration file.These hardcoded addresses should be moved to a configuration file for better maintainability and reusability across tests.
33-34: Use optional chaining for safer property access.Use optional chaining to safely access nested properties.
- if (tx.status && tx.status.confirmed) { + if (tx.status?.confirmed) {🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
49-51: Enhance error handling for network issues.The catch block should differentiate between network errors and other types of errors. Consider implementing exponential backoff for network-related errors.
} catch (error) { - console.log('Error checking transaction status:', error.message); + if (error.isAxiosError) { + console.log('Network error checking transaction status:', error.message); + // Implement exponential backoff here + } else { + console.log('Error checking transaction status:', error.message); + } }
115-117: Extract magic numbers into named constants.The fee amount and dust threshold should be extracted into named constants at the top of the file for better maintainability.
+const BITCOIN_TX_FEE = 1000; // satoshis +const BITCOIN_DUST_THRESHOLD = 546; // satoshis + // Add change output - const fee = 1000; + const fee = BITCOIN_TX_FEE; const change = totalInput - (amount * 100000000) - fee; - if (change > 546) { + if (change > BITCOIN_DUST_THRESHOLD) {
181-181: Extract hardcoded required balance into a constant.The required balance should be defined as a constant at the top of the file.
+const MINIMUM_STAKING_BALANCE = 0.01; // BTC + - const requiredBalance = 0.01; // set required balance to 0.01 BTC + const requiredBalance = MINIMUM_STAKING_BALANCE;
369-373: Extract magic indices into named constants.The balance array indices should be extracted into named constants for better readability and maintainability.
+const BALANCE_INDICES = { + CLIENT_CHAIN: 0, + STAKER_ADDRESS: 1, + TOKEN_ID: 2, + BALANCE: 3, + TOTAL_BALANCE: 7 +}; + - expect(finalBalance[0]).to.equal(CLIENT_CHAIN.BTC); - expect(ethers.hexlify(finalBalance[1])).to.equal(ethers.hexlify(staker.address)); - expect(ethers.hexlify(finalBalance[2])).to.equal(ethers.hexlify(BTC_ID)); - expect(finalBalance[3] - (initialBalance ? initialBalance[3] : 0)).to.equal(expectedIncrease); - expect(finalBalance[7] - (initialBalance ? initialBalance[7] : 0)).to.equal(expectedIncrease); + expect(finalBalance[BALANCE_INDICES.CLIENT_CHAIN]).to.equal(CLIENT_CHAIN.BTC); + expect(ethers.hexlify(finalBalance[BALANCE_INDICES.STAKER_ADDRESS])).to.equal(ethers.hexlify(staker.address)); + expect(ethers.hexlify(finalBalance[BALANCE_INDICES.TOKEN_ID])).to.equal(ethers.hexlify(BTC_ID)); + expect(finalBalance[BALANCE_INDICES.BALANCE] - (initialBalance ? initialBalance[BALANCE_INDICES.BALANCE] : 0)).to.equal(expectedIncrease); + expect(finalBalance[BALANCE_INDICES.TOTAL_BALANCE] - (initialBalance ? initialBalance[BALANCE_INDICES.TOTAL_BALANCE] : 0)).to.equal(expectedIncrease);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/compare-layouts.yml(1 hunks).github/workflows/forge-ci.yml(1 hunks)script/hardhat/deploy-and-setup-utxogateway.script.js(1 hunks)test/hardhat/integration/btc-stake-e2e.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/compare-layouts.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- script/hardhat/deploy-and-setup-utxogateway.script.js
🧰 Additional context used
📓 Learnings (1)
test/hardhat/integration/btc-stake-e2e.test.js (2)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/hardhat/integration/btc-stake-e2e.test.js:0-0
Timestamp: 2025-01-20T02:24:57.006Z
Learning: In the Exocore project's test suite, prefer using interval-based contract state querying over event listeners for monitoring state changes, as it provides better control and reliability.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/hardhat/integration/btc-stake-e2e.test.js:24-36
Timestamp: 2025-01-08T03:12:49.494Z
Learning: In Bitcoin transaction tests, prefer using test-level timeouts over function-level timeouts to handle long-running operations, as it provides a single point of control and avoids redundant timeout handling.
🪛 Biome (1.9.4)
test/hardhat/integration/btc-stake-e2e.test.js
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: setup / install
🔇 Additional comments (1)
test/hardhat/integration/btc-stake-e2e.test.js (1)
270-393: Well-implemented end-to-end test!The test suite is well-structured and thoroughly tests the Bitcoin staking functionality. It properly validates the initial conditions, handles the staking flow, and verifies the final state. The implementation follows best practices by using interval-based state querying and test-level timeouts.
adu-web3
requested review from
magj2006,
bwhour,
cloud8little,
cary007,
MaxMustermann2,
trestinlsd and
mikebraver
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
test/hardhat/integration/btc-stake-e2e.test.js (5)
24-24: Fix incorrect environment variable name in error message.The error message mentions
TEST_ACCOUNT_THREE_PRIVATE_KEYbut should referenceBITCOIN_FAUCET_PRIVATE_KEY.- throw new Error('BITCOIN_ESPLORA_API_URL or TEST_ACCOUNT_THREE_PRIVATE_KEY or BITCOIN_VAULT_ADDRESS is not set'); + throw new Error('BITCOIN_ESPLORA_API_URL or BITCOIN_FAUCET_PRIVATE_KEY or BITCOIN_VAULT_ADDRESS or BITCOIN_STAKER_PRIVATE_KEY is not set');
20-21: Ensure consistent numeric type handling.The constants
BITCOIN_TX_FEEandDUST_THRESHOLDare defined as BigInt but are used withNumber()in transaction outputs. This type conversion could potentially lead to precision loss for large values.- value: Number(amountSats) + value: parseInt(amountSats.toString())Also applies to: 110-111, 118-119, 226-227, 233-234
27-55: Improve error handling and use optional chaining.The function could benefit from better error handling and safer property access:
- Use optional chaining for
tx.statusaccess- Add specific error handling for network issues
async function waitForBitcoinConfirmation(txid, confirmations = 1) { console.log(`Waiting for ${confirmations} confirmation(s) for tx: ${txid}`); while (true) { try { const response = await axios.get(`${BITCOIN_ESPLORA_API_URL}/api/tx/${txid}`); const tx = response.data; - if (tx.status && tx.status.confirmed) { + if (tx.status?.confirmed) { const blockInfoResponse = await axios.get(`${BITCOIN_ESPLORA_API_URL}/api/blocks/tip/height`); const currentHeight = parseInt(blockInfoResponse.data); const txHeight = tx.status.block_height; const currentConfirmations = currentHeight - txHeight + 1; console.log(`Transaction confirmations: ${currentConfirmations}`); if (currentConfirmations >= confirmations) { console.log('Required confirmations reached'); return currentConfirmations; } } else { console.log('Transaction not yet confirmed...'); } } catch (error) { + if (error.code === 'ECONNREFUSED' || error.code === 'ECONNRESET') { + console.log('Network connection issue:', error.message); + } else if (error.response?.status === 404) { + console.log('Transaction not found'); + } else { console.log('Error checking transaction status:', error.message); + } } await new Promise(resolve => setTimeout(resolve, 1000)); } }🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
351-358: Extract balance verification logic to improve readability.The balance verification logic is complex and could be more readable if extracted into a helper function.
+ async function verifyBalance(finalBalance, initialBalance, expectedIncrease) { + expect(finalBalance[0]).to.equal(CLIENT_CHAIN.BTC); + expect(ethers.hexlify(finalBalance[1])).to.equal(ethers.hexlify(staker.address)); + expect(ethers.hexlify(finalBalance[2])).to.equal(ethers.hexlify(BTC_ID)); + expect(finalBalance[3] - (initialBalance ? initialBalance[3] : 0)).to.equal(expectedIncrease); + expect(finalBalance[7] - (initialBalance ? initialBalance[7] : 0)).to.equal(expectedIncrease); + } + expect(finalSuccess).to.be.true; const expectedIncrease = ethers.parseUnits('0.01', 8); - - expect(finalBalance[0]).to.equal(CLIENT_CHAIN.BTC); - expect(ethers.hexlify(finalBalance[1])).to.equal(ethers.hexlify(staker.address)); - expect(ethers.hexlify(finalBalance[2])).to.equal(ethers.hexlify(BTC_ID)); - expect(finalBalance[3] - (initialBalance ? initialBalance[3] : 0)).to.equal(expectedIncrease); - expect(finalBalance[7] - (initialBalance ? initialBalance[7] : 0)).to.equal(expectedIncrease); + await verifyBalance(finalBalance, initialBalance, expectedIncrease);
327-329: Define timeout constants for better maintainability.The test uses magic numbers for timeouts. These should be defined as constants at the top of the file.
+ const DEPOSIT_PROCESSING_TIMEOUT = 120000; // 2 minutes + const TEST_TIMEOUT = 300000; // 5 minutes + const timeout = setTimeout(() => { reject(new Error('Timeout waiting for deposit to be processed')); - }, 120000); + }, DEPOSIT_PROCESSING_TIMEOUT); -}).timeout(300000); +}).timeout(TEST_TIMEOUT);Also applies to: 377-377
script/hardhat/deploy-and-setup-utxogateway.script.js (4)
8-8: Consider making REQUIRED_PROOFS configurable.Hardcoding
REQUIRED_PROOFSmight not be suitable for different environments (testnet, mainnet). Consider making it configurable through environment variables or deployment configuration.-const REQUIRED_PROOFS = 3; +const REQUIRED_PROOFS = process.env.REQUIRED_PROOFS || 3;
39-42: Use optional chaining for cleaner deployment checks.The deployment check can be simplified using optional chaining.
- if (deployedContracts.exocore.utxoGateway) { + if (deployedContracts.exocore?.utxoGateway) { console.log("Using existing UTXOGateway deployment:", deployedContracts.exocore.utxoGateway); return; }
44-81: Add deployment verification and gas estimation.Consider adding these safety checks for more robust deployments:
- Gas estimation before deployment
- Contract code verification after deployment
const UTXOGatewayFactory = await ethers.getContractFactory("UTXOGateway"); + // Estimate gas for deployment + const deploymentGas = await UTXOGatewayFactory.getDeployTransaction().estimateGas(); + console.log("Estimated gas for deployment:", deploymentGas.toString()); + const utxoGatewayLogic = await UTXOGatewayFactory.connect(deployer).deploy(); await utxoGatewayLogic.waitForDeployment(); + // Verify contract code was deployed + const code = await ethers.provider.getCode(await utxoGatewayLogic.getAddress()); + assert(code !== "0x", "Contract code not deployed");🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
86-87: Add timeout for transaction confirmations.Add a timeout to prevent the script from hanging indefinitely while waiting for transactions.
- await authTx.wait(); + await authTx.wait(3); // Wait for 3 confirmations with default timeout - await activateTx.wait(); + await activateTx.wait(3); // Wait for 3 confirmations with default timeoutAlso applies to: 91-92
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
script/deployments/utxo-gateway-exocore_localnet.json(1 hunks)script/hardhat/deploy-and-setup-utxogateway.script.js(1 hunks)test/hardhat/integration/btc-stake-e2e.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- script/deployments/utxo-gateway-exocore_localnet.json
🧰 Additional context used
📓 Learnings (2)
script/hardhat/deploy-and-setup-utxogateway.script.js (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: script/hardhat/deploy-and-setup-utxogateway.script.js:84-85
Timestamp: 2025-01-22T06:14:09.961Z
Learning: In the Exocore project, direct contract state queries are preferred over event verification for validating contract operations, as state is the source of truth.
test/hardhat/integration/btc-stake-e2e.test.js (2)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/hardhat/integration/btc-stake-e2e.test.js:0-0
Timestamp: 2025-01-20T02:24:57.006Z
Learning: In the Exocore project's test suite, prefer using interval-based contract state querying over event listeners for monitoring state changes, as it provides better control and reliability.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/hardhat/integration/btc-stake-e2e.test.js:24-36
Timestamp: 2025-01-08T03:12:49.494Z
Learning: In Bitcoin transaction tests, prefer using test-level timeouts over function-level timeouts to handle long-running operations, as it provides a single point of control and avoids redundant timeout handling.
🪛 Biome (1.9.4)
script/hardhat/deploy-and-setup-utxogateway.script.js
[error] 53-53: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
test/hardhat/integration/btc-stake-e2e.test.js
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: setup / install
🔇 Additional comments (3)
script/hardhat/deploy-and-setup-utxogateway.script.js (3)
94-108: LGTM! Comprehensive setup verification.The verification includes all necessary checks:
- Gateway authorization
- Chain registration
- Required proofs configuration
- Witness authorization
110-145: LGTM! Complete deployment information persistence.The deployment info captures all necessary details and follows the project's file operation practices.
158-169: LGTM! Proper script execution handling.The script handles both direct execution and module import scenarios with appropriate error handling and exit codes.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
docs/bit-staking-e2e-testing.md (3)
7-10: Specify version requirements for Docker and Docker Compose.Consider adding minimum version requirements for Docker and Docker Compose to ensure compatibility.
-- Docker and Docker Compose +- Docker (v20.10+) and Docker Compose (v2.0+)
100-100: Fix capitalization in NOTICE keyword.-NoTICE: We'd better set `MIN_CONFIRMATIONS` to 1 to avoid timeouts +NOTICE: We'd better set `MIN_CONFIRMATIONS` to 1 to avoid timeouts
119-119: Add verification and troubleshooting sections.Consider adding the following sections to improve the documentation:
- Verification steps to confirm successful setup at each stage
- Common issues and troubleshooting guide
- Clean-up instructions for stopping services
Example sections to add:
## Verification Steps ### Verify Bitcoin Node Setup 1. Check if the node is running: `bitcoin-cli getblockcount` 2. Verify faucet balance: `bitcoin-cli getbalance` ### Verify Exocore Node Setup 1. Check node synchronization 2. Verify RPC connectivity ## Troubleshooting Guide ### Common Issues 1. Docker container fails to start - Solution: Check port conflicts 2. Transaction timeout - Solution: Adjust MIN_CONFIRMATIONS ## Cleanup 1. Stop Bitcoin node: `docker-compose down` 2. Stop Exocore node: `./local_node.sh stop` 3. Stop UTXO Bridge: `docker-compose down`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/bit-staking-e2e-testing.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/bit-staking-e2e-testing.md
[uncategorized] ~23-~23: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... send test token to faucet and activate auto mining by running script: ```bash ./start-reg...
(AUTO_HYPHEN)
[style] ~71-~71: Consider using a different verb to strengthen your wording.
Context: ... running the deployment command, please make sure the required environment variables are ...
(MAKE_SURE_ENSURE)
[uncategorized] ~117-~117: Possible missing preposition found.
Context: ...fore running the test, please make sure confiture the test environment variables in your ...
(AI_HYDRA_LEO_MISSING_TO)
🪛 Gitleaks (8.21.2)
docs/bit-staking-e2e-testing.md
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: setup / install
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
docs/btc-staking-e2e-testing.md (4)
7-10: Consider specifying version requirements for all prerequisites.For better reproducibility, consider adding specific version requirements for Docker, Docker Compose, and npm/yarn.
- Docker and Docker Compose +- Docker (v20.10+) and Docker Compose (v2.0+) - Node.js (v16+) -- npm or yarn +- npm (v8.0+) or yarn (v1.22+) - Git
54-55: Consider using stable releases instead of develop branch.Using the develop branch may lead to inconsistent behavior as it contains unreleased changes. Consider recommending a stable release version instead.
-# Switch to develop branch -git checkout develop +# Checkout the latest stable release +git checkout $(git describe --tags `git rev-list --tags --max-count=1`)
126-126: Fix typo in notice message.Correct the capitalization in "NoTICE".
-NoTICE: We'd better set `MIN_CONFIRMATIONS` to 1 to avoid timeouts +NOTICE: We'd better set `MIN_CONFIRMATIONS` to 1 to avoid timeouts
23-23: Fix grammatical issues throughout the document.Several grammatical improvements are needed:
- Line 23: Add hyphen in "auto-mining"
- Line 43: Add article "a" before "known private key"
- Line 71: Hyphenate "set-up" when used as a modifier
- Line 109: Add "the" before "address"
- Line 153: Add "the" before "staker's"
Also applies to: 43-43, 71-71, 109-109, 153-153
🧰 Tools
🪛 LanguageTool
[uncategorized] ~23-~23: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... send test token to faucet and activate auto mining by running script: ```bash ./start-reg...(AUTO_HYPHEN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/btc-staking-e2e-testing.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/btc-staking-e2e-testing.md
[uncategorized] ~23-~23: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... send test token to faucet and activate auto mining by running script: ```bash ./start-reg...
(AUTO_HYPHEN)
[uncategorized] ~43-~43: You might be missing the article “a” here.
Context: ...ould be sent to the faucet address with known private key ### Start Exocore Node 1....
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~71-~71: When ‘set-up’ is used as a noun or modifier, it needs to be hyphenated.
Context: ...at.config.js` with the URL path of your set up Exocore node, and also set the private ...
(VERB_NOUN_CONFUSION)
[uncategorized] ~109-~109: You might be missing the article “the” here.
Context: ...` 2. Configure the bridge service with address of deployed UTXOGateway contract, witne...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~153-~153: You might be missing the article “the” here.
Context: ...t to be confirmed, and finally checking staker's balance on Exocore.
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Gitleaks (8.21.2)
docs/btc-staking-e2e-testing.md
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: setup / install
🔇 Additional comments (2)
docs/btc-staking-e2e-testing.md (2)
23-27: Clarify script location and initial setup.The documentation jumps directly to running the script without mentioning its location or initial setup steps. Please add information about where to find or create the
start-regtest.shscript.🧰 Tools
🪛 LanguageTool
[uncategorized] ~23-~23: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... send test token to faucet and activate auto mining by running script: ```bash ./start-reg...(AUTO_HYPHEN)
36-37:⚠️ Potential issueSecurity: Avoid exposing private keys in documentation.
Even for testing environments, it's recommended to avoid hardcoding private keys in documentation. Consider using environment variables or configuration files.
- Private Key: 0xee01cfc3f08cdb020064f31ff1a993aa9ecc1d38e684665742faa705685532a6 + Private Key: <FAUCET_PRIVATE_KEY> # Set this in your .env file⛔ Skipped due to learnings
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#132 File: docs/bit-staking-e2e-testing.md:36-37 Timestamp: 2025-01-24T02:05:14.571Z Learning: In the Bitcoin regtest testing environment, it is acceptable and preferred to hardcode test faucet private keys and addresses in documentation for convenience and reproducibility of tests.🧰 Tools
🪛 Gitleaks (8.21.2)
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
docs/btc-staking-e2e-testing.md (5)
7-10: Enhance prerequisites with specific version requirements.Add version requirements for Docker and Docker Compose to ensure compatibility.
-Docker and Docker Compose +Docker (v20.10+) and Docker Compose (v2.0+)
23-23: Fix hyphenation in technical term.Correct the hyphenation of "auto-mining" for clarity.
-Start the Bitcoin regtest node, esplora, send test token to faucet and activate auto mining by running script: +Start the Bitcoin regtest node, esplora, send test token to faucet and activate auto-mining by running script:🧰 Tools
🪛 LanguageTool
[uncategorized] ~23-~23: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... send test token to faucet and activate auto mining by running script: ```bash ./start-reg...(AUTO_HYPHEN)
54-55: Use specific commit hash or tag instead of branch name.For reproducibility, consider using a specific commit hash or tag instead of the develop branch.
-# Switch to develop branch -git checkout develop +# Switch to specific version +git checkout <specific-commit-hash-or-tag>
109-109: Fix grammar in configuration instruction.Add missing article "the" for better readability.
-Configure the bridge service with address of deployed UTXOGateway contract, witness address and its private key and other required parameters: +Configure the bridge service with the address of deployed UTXOGateway contract, witness address and its private key and other required parameters:🧰 Tools
🪛 LanguageTool
[uncategorized] ~109-~109: You might be missing the article “the” here.
Context: ...` 2. Configure the bridge service with address of deployed UTXOGateway contract, witne...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
153-153: Fix grammar in final sentence.Add missing article "the" for better readability.
-This test would simulate the process of building a valid Bitcoin deposit transaction, broadcasting it to the Bitcoin network and waiting for it to be confirmed, and finally checking staker's balance on Exocore. +This test would simulate the process of building a valid Bitcoin deposit transaction, broadcasting it to the Bitcoin network and waiting for it to be confirmed, and finally checking the staker's balance on Exocore.🧰 Tools
🪛 LanguageTool
[uncategorized] ~153-~153: You might be missing the article “the” here.
Context: ...t to be confirmed, and finally checking staker's balance on Exocore.(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/btc-staking-e2e-testing.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/btc-staking-e2e-testing.md
[uncategorized] ~23-~23: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... send test token to faucet and activate auto mining by running script: ```bash ./start-reg...
(AUTO_HYPHEN)
[uncategorized] ~43-~43: You might be missing the article “a” here.
Context: ...ould be sent to the faucet address with known private key ### Start Exocore Node 1....
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~71-~71: When ‘set-up’ is used as a noun or modifier, it needs to be hyphenated.
Context: ...at.config.js` with the URL path of your set up Exocore node, and also set the private ...
(VERB_NOUN_CONFUSION)
[uncategorized] ~109-~109: You might be missing the article “the” here.
Context: ...` 2. Configure the bridge service with address of deployed UTXOGateway contract, witne...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~153-~153: You might be missing the article “the” here.
Context: ...t to be confirmed, and finally checking staker's balance on Exocore.
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Gitleaks (8.21.2)
docs/btc-staking-e2e-testing.md
36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: setup / install
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
docs/ClientChainGateway.md (6)
1-10: Enhance overview with additional context.Consider adding:
- A brief explanation of LayerZero for non-technical readers
- Definitions of LST (Liquid Staking Token) and NST (Native Staking Token) when first introduced
# ClientChainGateway Documentation -The ClientChainGateway serves as the primary interface for stakers to interact with the Exocore network. It functions as a LayerZero application capable of cross-chain messaging and manages both Vault and Capsule contracts. +The ClientChainGateway serves as the primary interface for stakers to interact with the Exocore network. It functions as a LayerZero application (a protocol for trustless cross-chain messaging) and manages both Vault and Capsule contracts. ## Overview ClientChainGateway implements two types of restaking: -1. LST (Liquid Staking Token) Restaking -2. NST (Native Staking Token) Restaking +1. LST (Liquid Staking Token) Restaking - Allows staking of tokens that represent staked assets (e.g., stETH) +2. NST (Native Staking Token) Restaking - Enables direct staking of native tokens (e.g., ETH)
11-51: Enhance LST functions documentation with additional details.Consider adding:
- Parameter descriptions for each function
- Error conditions and handling
- Gas considerations for cross-chain operations
Example enhancement for the
depositfunction:- `deposit(address token, uint256 amount)` + Parameters: + - `token`: Address of the whitelisted token to deposit + - `amount`: Amount of tokens to deposit (in token's smallest unit) - Deposits whitelisted tokens into Exocore - Locks tokens in a Vault contract - Sends cross-chain message to Exocore for accounting - Always considered successful on Exocore side + Error conditions: + - Reverts if token is not whitelisted + - Reverts if amount exceeds user's balance + - Reverts if user has not approved sufficient allowance + Gas considerations: + - Requires additional gas for LayerZero message🧰 Tools
🪛 LanguageTool
[uncategorized] ~47-~47: Possible missing preposition found.
Context: ...ount, address recipient)` - Transfers unlocked tokens from Vault to recipient - Must...(AI_HYDRA_LEO_MISSING_OF)
47-47: Fix grammatical issue in withdrawal operation description.Add the preposition "to" for better clarity.
-- `withdrawPrincipal(address token, uint256 amount, address recipient)` +- `withdrawPrincipal(address token, uint256 amount, address recipient)` - - Transfers unlocked tokens from Vault to recipient + - Transfers unlocked tokens from the Vault to the recipient🧰 Tools
🪛 LanguageTool
[uncategorized] ~47-~47: Possible missing preposition found.
Context: ...ount, address recipient)` - Transfers unlocked tokens from Vault to recipient - Must...(AI_HYDRA_LEO_MISSING_OF)
52-84: Add security considerations and clarify beacon chain interactions.Consider adding:
- Security considerations for beacon chain proofs verification
- Detailed explanation of ExoCapsule's role in withdrawal credentials
- Minimum staking requirements and limitations
Example enhancement for the
verifyAndDepositNativeStakefunction:- `verifyAndDepositNativeStake(bytes32[] validatorContainer, BeaconChainProofs.ValidatorContainerProof proof)` + Security considerations: + - Proof verification ensures validator legitimacy + - Prevents unauthorized claiming of validator balances + - Requires recent state root for proof freshness - Verifies beacon chain proof of withdrawal credentials - Confirms validator is using correct ExoCapsule - Sends message to Exocore to account for validator balance - Required for NST restaking activation + Proof requirements: + - Must be generated from a finalized beacon chain state + - Must include validator's withdrawal credentials + - Must prove validator's effective balance
85-109: Enhance workflows with timing and failure handling.Consider adding:
- Estimated time frames for cross-chain operations
- Failure scenarios and recovery steps
- Gas estimation for each step
Example enhancement for LST Withdrawal workflow:
### LST Withdrawal 1. Call `claimPrincipalFromExocore` 2. Wait for cross-chain message confirmation 3. Call `withdrawPrincipal` to receive tokens + +Timing and considerations: +- Cross-chain message confirmation typically takes 10-15 minutes +- If message fails, retry using the LayerZero recovery system +- Ensure sufficient gas for both transactions + +Failure scenarios: +1. Cross-chain message timeout + - Check message status on LayerZero Explorer + - Retry if necessary +2. Insufficient gas for message delivery + - Increase gas limit and retry🧰 Tools
🪛 LanguageTool
[duplication] ~99-~99: Possible typo: you repeated a word.
Context: ...to receive tokens ### NST Restaking 1. CallcreateExoCapsule2. Callstaketo become validator 3. Call `ve...(ENGLISH_WORD_REPEAT_RULE)
110-114: Expand notes with additional resources and timelines.Consider adding:
- Links to LayerZero fee documentation
- Timeline for enabling reward-related functions
- Links to beacon chain proof generation tools
## Notes - All cross-chain operations require LayerZero fees - Reward-related functions are currently disabled - NST operations require valid beacon chain proofs - Delegation requires registered operators on Exocore + +Additional resources: +- LayerZero fee calculator: [URL] +- Beacon chain proof generation guide: [URL] +- List of registered operators: [URL] + +Upcoming features: +- Reward-related functions planned for Q2 2025 +- Automated proof generation tools in development
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/ClientChainGateway.md(1 hunks)src/libraries/SignatureVerifier.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libraries/SignatureVerifier.sol
🧰 Additional context used
🪛 LanguageTool
docs/ClientChainGateway.md
[uncategorized] ~47-~47: Possible missing preposition found.
Context: ...ount, address recipient)` - Transfers unlocked tokens from Vault to recipient - Must...
(AI_HYDRA_LEO_MISSING_OF)
[duplication] ~99-~99: Possible typo: you repeated a word.
Context: ...to receive tokens ### NST Restaking 1. Call createExoCapsule 2. Call stake to become validator 3. Call `ve...
(ENGLISH_WORD_REPEAT_RULE)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/storage/UTXOGatewayStorage.sol (2)
129-132: Optional parameterization of proof timeout.
The constant PROOF_TIMEOUT is set to 1 day. While this is a reasonable default, you may consider making it configurable via an owner setter function, allowing more flexibility if the off-chain environment changes.
476-493: Additional validation for operator addresses.
The isValidOperatorAddress method only checks length and prefix. Although this is a good heuristic, consider clarifying in documentation that it is not a full bech32 validation. If stricter checks become necessary, you could integrate a more comprehensive bech32 library.docs/btc-staking-e2e-testing.md (4)
23-23: Fix hyphenation in auto-mining.Add a hyphen to "auto-mining" for correct compound word formation.
-2. Start the Bitcoin regtest node, esplora, send test token to faucet and activate auto mining by running script: +2. Start the Bitcoin regtest node, esplora, send test token to faucet and activate auto-mining by running script:🧰 Tools
🪛 LanguageTool
[uncategorized] ~23-~23: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... send test token to faucet and activate auto mining by running script: ```bash ./start-reg...(AUTO_HYPHEN)
43-43: Add missing article.Add the article "a" before "known private key".
-NOTICE: Some amount of test BTC would be sent to the faucet address with known private key +NOTICE: Some amount of test BTC would be sent to the faucet address with a known private key🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: You might be missing the article “a” here.
Context: ...ould be sent to the faucet address with known private key ### Start Exocore Node 1....(AI_EN_LECTOR_MISSING_DETERMINER_A)
71-71: Fix hyphenation in set-up.Add a hyphen to "set-up" when used as a modifier.
-Before running the deployment command, please replace the URL path of `exocore_localnet` in `hardhat.config.js` with the URL path of your set up Exocore node +Before running the deployment command, please replace the URL path of `exocore_localnet` in `hardhat.config.js` with the URL path of your set-up Exocore node🧰 Tools
🪛 LanguageTool
[uncategorized] ~71-~71: When ‘set-up’ is used as a noun or modifier, it needs to be hyphenated.
Context: ...at.config.js` with the URL path of your set up Exocore node, and also set the private ...(VERB_NOUN_CONFUSION)
153-153: Add missing article.Add the article "the" before "staker's balance".
-This test would simulate the process of building a valid Bitcoin deposit transaction, broadcasting it to the Bitcoin network and waiting for it to be confirmed, and finally checking staker's balance on Exocore. +This test would simulate the process of building a valid Bitcoin deposit transaction, broadcasting it to the Bitcoin network and waiting for it to be confirmed, and finally checking the staker's balance on Exocore.🧰 Tools
🪛 LanguageTool
[uncategorized] ~153-~153: You might be missing the article “the” here.
Context: ...t to be confirmed, and finally checking staker's balance on Exocore.(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/ClientChainGateway.md(1 hunks)docs/btc-staking-e2e-testing.md(1 hunks)hardhat.config.js(2 hunks)src/core/UTXOGateway.sol(1 hunks)src/storage/UTXOGatewayStorage.sol(1 hunks)test/foundry/unit/UTXOGateway.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/foundry/unit/UTXOGateway.t.sol
🧰 Additional context used
📓 Learnings (2)
src/core/UTXOGateway.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/core/UTXOGateway.sol:407-424
Timestamp: 2025-01-08T03:15:08.317Z
Learning: The `onlyAuthorizedWitness` modifier checking if `msg.sender` is an authorized witness provides sufficient access control for functions that should only be called by witnesses in the UTXOGateway contract.
src/storage/UTXOGatewayStorage.sol (2)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/storage/UTXOGatewayStorage.sol:457-462
Timestamp: 2025-01-20T02:28:24.440Z
Learning: In the UTXOGateway contract, Token.NONE cannot be registered in outboundRegistry because registration requires a valid ClientChainID and goes through _registerAddress function which has multiple validations. The clientChainId parameter comes from validated sources (stake messages, etc.), making additional Token.NONE checks in the isRegistered modifier redundant.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/storage/UTXOGatewayStorage.sol:457-462
Timestamp: 2025-01-20T02:28:24.440Z
Learning: In the UTXOGateway contract, Token.NONE cannot be registered in outboundRegistry due to validations during the registration process, making additional Token.NONE checks in the isRegistered modifier redundant.
🪛 LanguageTool
docs/ClientChainGateway.md
[duplication] ~111-~111: Possible typo: you repeated a word.
Context: ...to receive tokens ### NST Restaking 1. Call createExoCapsule 2. Call stake to become validator 3. Call `ve...
(ENGLISH_WORD_REPEAT_RULE)
docs/btc-staking-e2e-testing.md
[uncategorized] ~23-~23: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... send test token to faucet and activate auto mining by running script: ```bash ./start-reg...
(AUTO_HYPHEN)
[uncategorized] ~43-~43: You might be missing the article “a” here.
Context: ...ould be sent to the faucet address with known private key ### Start Exocore Node 1....
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~71-~71: When ‘set-up’ is used as a noun or modifier, it needs to be hyphenated.
Context: ...at.config.js` with the URL path of your set up Exocore node, and also set the private ...
(VERB_NOUN_CONFUSION)
[uncategorized] ~153-~153: You might be missing the article “the” here.
Context: ...t to be confirmed, and finally checking staker's balance on Exocore.
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (10)
src/storage/UTXOGatewayStorage.sol (3)
16-20: Enumerations align with cross-chain IDs effectively.
It’s good that you’ve included comments linking each token enum value to its corresponding client chain ID. This ensures clarity between enumerations and helps future maintainers understand the purpose of each numeric value.
179-193: Cross-referencing inbound & outbound registries.
The inboundRegistry and outboundRegistry mappings efficiently track the associations between client addresses and Exocore addresses. This bidirectional linkage is crucial for deposit/withdraw flows. No issues found here; the approach looks solid.
459-464: No additional checks for Token.NONE are necessary.
Given the preexisting registration validations, an explicit Token.NONE check is not required in the isRegistered modifier. This is consistent with prior learnings that Token.NONE cannot be registered, so the current code is correct and avoids redundant checks.src/core/UTXOGateway.sol (5)
50-73: Initialization flow seems robust.
The initialize function properly sets up ownership, witnesses, and requiredProofs, enforcing validity checks. Using __Pausable_init_unchained, __ReentrancyGuard_init_unchained, and _transferOwnership ensures safe upgradeable initialization. Good approach.
94-103: Single path for chain registration.
activateStakingForClientChain cleanly routes to _registerOrUpdateClientChain and _registerOrUpdateToken. This is straightforward and helps keep chain-specific logic organized.
178-226: Robust witness-proof flow with potential reentrancy guarded.
submitProofForStakeMsg safeguards consensus logic, ensures each witness can only submit once per proof window, and properly finalizes the transaction after enough proofs. The use of nonReentrant and re-checking the transaction status to guard reentrancy is commendable.
257-276: Delegation success is validated but not forced.
delegateTo calls _delegate and checks success, reverting if delegation fails. This is consistent with usage scenarios where the operator must be valid on Exocore. The isRegistered modifier ensures depositor addresses are recognized.
829-866: Fee deduction logic in _processStakeMsg.
Deducting the fee from the staker’s amount (bridgeFeeRate) before deposit ensures that the deposit operation always represents the net amount. Emitting DelegationFailedForStake if delegations fail is a nice touch to track partial successes.docs/ClientChainGateway.md (1)
1-128: Documentation is comprehensive and user-friendly.
Your ClientChainGateway docs clearly explain both LST and NST restaking flows, including deposit, delegation, staking, and withdrawal steps. The reference to cross-chain relay fees offers practical guidance. No repeated-word issues are apparent despite the static analysis hint. Likely a false positive.🧰 Tools
🪛 LanguageTool
[duplication] ~111-~111: Possible typo: you repeated a word.
Context: ...to receive tokens ### NST Restaking 1. CallcreateExoCapsule2. Callstaketo become validator 3. Call `ve...(ENGLISH_WORD_REPEAT_RULE)
hardhat.config.js (1)
8-8: LGTM! Version update aligns with foundry.toml.The Solidity version has been updated to match the foundry.toml configuration.
adu-web3
force-pushed
the
btc-gateway
branch
from
69e27c0 to
f476db7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (15)
src/storage/UTXOGatewayStorage.sol (1)
16-19: Ensure clarity betweenToken.NONEand other tokens.
DefiningToken.NONEas a zero value can be useful for undefined or uninitialized tokens, but be aware that future tokens in the enum may shift indexing. Consider documenting or adding references that clarify thatToken.NONEis never registered.src/core/UTXOGateway.sol (2)
592-610: Re-examine the edge case for removing the second-to-last witness.
If there are exactly two witnesses and one is removed, you will have one witness left, which is allowed. Confirm that there are no situations where the system could become locked if some other invariants rely on having at least two witnesses for consensus.
846-865: Handle multi-chain scenarios with varying fees.
The_processStakeMsgfunction applies a flat fee rate for all tokens. If future expansions allow multiple chain/tokens with different fee schedules, consider storing a per-chain fee structure instead of a single globalbridgeFeeRate.test/mocks/AssetsMock.sol (2)
77-86: Provide detailed revert messages for unregistered asset checks.
Currently, it returns(false, 0)when disallowing a withdrawal. Consider reverting withrequire(...)or returning explicit error codes to help testers identify the reason for withdrawal failure.
219-237: Consider merging balances for principal vs. reward.
The returnedStakerBalanceuses the same value for different fields (principal,unclaimedReward, etc.) in the mock. If your real contract logic differentiates them, ensure tests properly reflect those differences.Do you need help adding multiple internal counters to simulate real-world scenarios for principal vs. reward balances?
script/hardhat/deploy-and-setup-utxogateway.script.js (4)
10-10: Consider extracting CREATE3 initialization code.The hardcoded CREATE3 initialization code makes the script harder to maintain. Consider moving it to a separate file or loading it from the contract's artifacts.
35-43: Add gas token balance validation.Consider validating that the deployer has enough gas tokens before attempting transfers.
+ const deployerBalance = await ethers.provider.getBalance(deployer.address); + const requiredBalance = ethers.parseEther("3"); // 1 ether for each account + if (deployerBalance < requiredBalance) { + throw new Error(`Insufficient deployer balance. Need ${ethers.formatEther(requiredBalance)} ETH, have ${ethers.formatEther(deployerBalance)} ETH`); + } + for (const account of [deployer, owner, witness1]) {
67-68: Add bytecode verification for CREATE3 factory.Consider verifying that the deployed CREATE3 factory has the expected bytecode.
// Verify deployment const newCode = await ethers.provider.getCode(CREATE3_DESTINATION); - if (newCode === "0x") { + if (newCode === "0x" || newCode !== CREATE3_INIT_CODE) { throw new Error("Failed to deploy CREATE3 factory"); }
178-185: Add file write permission validation.Consider validating file write permissions before attempting to save deployment info.
const deploymentsDir = path.join(__dirname, '../deployments'); + try { + // Test write permissions + const testFile = path.join(deploymentsDir, '.write-test'); + fs.writeFileSync(testFile, ''); + fs.unlinkSync(testFile); + } catch (error) { + throw new Error(`No write permissions in ${deploymentsDir}: ${error.message}`); + } + if (!fs.existsSync(deploymentsDir)) {test/hardhat/integration/btc-stake-e2e.test.js (1)
88-101: Consider sorting UTXOs for optimal input selection.Sort UTXOs by value to minimize the number of inputs needed and optimize change outputs.
// Add inputs until we have enough for amount + fee let totalInputSats = 0n; + const sortedUtxos = utxos.sort((a, b) => b.value - a.value); // Sort descending - for (const utxo of utxos) { + for (const utxo of sortedUtxos) {package.json (1)
18-37: Consider organizing dependencies by purpose.The devDependencies section could benefit from organization by grouping related packages (e.g., Bitcoin-related, Ethereum-related, testing tools).
"devDependencies": { + # Bitcoin-related "@cosmjs/proto-signing": "^0.32.4", "@cosmjs/stargate": "^0.32.2", + "bitcoinjs-lib": "^6.1.5", + "ecpair": "^2.1.0", + "tiny-secp256k1": "^2.2.3", + + # Ethereum/Testing "@evmos/address-converter": "^0.1.9", "@evmos/evmosjs": "github:evmos/evmosjs#main", "@evmos/proto": "^0.2.1", "@nomicfoundation/hardhat-foundry": "^1.1.3", "@nomicfoundation/hardhat-toolbox": "^5.0.0", + "hardhat": "^2.22.17", + "mocha": "^10.2.0", + "solhint": "^5.0.1", + + # Utilities "axios": "^1.7.9", "bip39": "^3.1.0", "dotenv": "^16.4.7", "esm": "^3.2.25", "ethereumjs-wallet": "^1.0.2",docs/ClientChainGateway.md (1)
122-128: Add security considerations section.Consider adding a dedicated security section that covers:
- Risks associated with cross-chain operations
- Importance of proper key management
- Potential attack vectors
- Best practices for operators
docs/btc-staking-e2e-testing.md (2)
7-10: Add specific version requirements.Consider adding minimum version requirements for each prerequisite:
- Docker version X.X+
- Docker Compose version X.X+
- Node.js version 16.x+
- npm version X.X+ or yarn version X.X+
89-96: Add verification steps after deployment.Consider adding steps to verify the successful deployment and setup:
- How to check if the contract is properly authorized
- How to verify witness setup
- How to confirm BTC staking activation
src/utils/Dependencies.sol (1)
4-6: Add pragma directive to silence unused import warning.Since this file is specifically for making CREATE3Factory available to scripts, consider adding a pragma directive to silence the unused import warning:
import {CREATE3Factory} from "create3-factory/src/CREATE3Factory.sol"; + +/* solhint-disable no-unused-import */🧰 Tools
🪛 GitHub Check: lint
[failure] 4-4:
imported name CREATE3Factory is not used🪛 GitHub Actions: Solhint
[error] 4-4: imported name CREATE3Factory is not used no-unused-import
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (72)
.github/workflows/compare-layouts.yml(1 hunks).github/workflows/forge-ci.yml(1 hunks).gitignore(1 hunks).gitmodules(1 hunks)docs/ClientChainGateway.md(1 hunks)docs/btc-staking-e2e-testing.md(1 hunks)hardhat.config.js(2 hunks)lib/create3-factory(1 hunks)package.json(1 hunks)remappings.txt(0 hunks)script/10_DeployExocoreGatewayOnly.s.sol(3 hunks)script/11_SetPeers.s.sol(2 hunks)script/12_RedeployClientChainGateway.s.sol(3 hunks)script/13_DepositValidator.s.sol(2 hunks)script/14_CorrectBootstrapErrors.s.sol(4 hunks)script/15_DeploySafeMulstisigWallet.s.sol(1 hunks)script/16_UpgradeExoCapsule.s.sol(1 hunks)script/17_WithdrawalValidator.s.sol(4 hunks)script/18_SimulateReceive.s.sol(1 hunks)script/19_DeployFaucet.s.sol(1 hunks)script/1_Prerequisites.s.sol(2 hunks)script/2_DeployBoth.s.sol(3 hunks)script/3_Setup.s.sol(2 hunks)script/4_Deposit.s.sol(2 hunks)script/5_Withdraw.s.sol(2 hunks)script/6_CreateExoCapsule.s.sol(3 hunks)script/7_DeployBootstrap.s.sol(3 hunks)script/8_RegisterValidatorsAndDelegate.s.sol(1 hunks)script/9_ExtendBootstrapTime.s.sol(1 hunks)script/BaseScript.sol(1 hunks)script/TestPrecompileErrorFixed.s.sol(0 hunks)script/TestPrecompileErrorFixed_Deploy.s.sol(0 hunks)script/TokenTransfer.s.sol(0 hunks)script/deployBeaconOracle.s.sol(1 hunks)script/hardhat/deploy-and-setup-utxogateway.script.js(1 hunks)slither.config.json(1 hunks)src/core/BaseRestakingController.sol(1 hunks)src/core/ClientChainGateway.sol(1 hunks)src/core/ExocoreGateway.sol(1 hunks)src/core/RewardVault.sol(1 hunks)src/core/UTXOGateway.sol(1 hunks)src/interfaces/IClientChainGateway.sol(1 hunks)src/interfaces/IExocoreGateway.sol(1 hunks)src/interfaces/precompiles/IAssets.sol(6 hunks)src/libraries/Errors.sol(1 hunks)src/libraries/ExocoreBytes.sol(1 hunks)src/libraries/SignatureVerifier.sol(1 hunks)src/lzApp/OAppCoreUpgradeable.sol(1 hunks)src/lzApp/OAppReceiverUpgradeable.sol(1 hunks)src/lzApp/OAppSenderUpgradeable.sol(1 hunks)src/storage/UTXOGatewayStorage.sol(1 hunks)src/utils/Dependencies.sol(1 hunks)test/foundry/Delegation.t.sol(1 hunks)test/foundry/DepositThenDelegateTo.t.sol(1 hunks)test/foundry/DepositWithdrawPrincipal.t.sol(1 hunks)test/foundry/ExocoreDeployer.t.sol(1 hunks)test/foundry/Governance.t.sol(1 hunks)test/foundry/TvlLimits.t.sol(1 hunks)test/foundry/unit/ClientChainGateway.t.sol(1 hunks)test/foundry/unit/ExoCapsule.t.sol(0 hunks)test/foundry/unit/ExocoreGateway.t.sol(2 hunks)test/foundry/unit/UTXOGateway.t.sol(1 hunks)test/hardhat/client-chain/deploy_fixture.js(0 hunks)test/hardhat/client-chain/gateway.test.js(0 hunks)test/hardhat/exocore/deploy_fixture.js(0 hunks)test/hardhat/exocore/gateway.test.js(0 hunks)test/hardhat/integration/btc-stake-e2e.test.js(1 hunks)test/hardhat/integration/btc-stake.test.js(1 hunks)test/mocks/AssetsMock.sol(6 hunks)test/mocks/DelegationMock.sol(4 hunks)test/mocks/ExocoreGatewayMock.sol(1 hunks)test/mocks/NonShortCircuitLzEndpointMock.sol(0 hunks)
💤 Files with no reviewable changes (10)
- test/hardhat/client-chain/gateway.test.js
- test/hardhat/exocore/gateway.test.js
- remappings.txt
- test/foundry/unit/ExoCapsule.t.sol
- script/TestPrecompileErrorFixed_Deploy.s.sol
- test/mocks/NonShortCircuitLzEndpointMock.sol
- script/TestPrecompileErrorFixed.s.sol
- test/hardhat/client-chain/deploy_fixture.js
- script/TokenTransfer.s.sol
- test/hardhat/exocore/deploy_fixture.js
✅ Files skipped from review due to trivial changes (1)
- lib/create3-factory
🚧 Files skipped from review as they are similar to previous changes (49)
- .gitignore
- script/9_ExtendBootstrapTime.s.sol
- .github/workflows/compare-layouts.yml
- script/19_DeployFaucet.s.sol
- .github/workflows/forge-ci.yml
- script/3_Setup.s.sol
- script/11_SetPeers.s.sol
- script/8_RegisterValidatorsAndDelegate.s.sol
- script/6_CreateExoCapsule.s.sol
- script/16_UpgradeExoCapsule.s.sol
- src/libraries/ExocoreBytes.sol
- script/4_Deposit.s.sol
- src/lzApp/OAppReceiverUpgradeable.sol
- src/lzApp/OAppCoreUpgradeable.sol
- script/18_SimulateReceive.s.sol
- script/2_DeployBoth.s.sol
- src/core/BaseRestakingController.sol
- test/foundry/unit/ExocoreGateway.t.sol
- test/hardhat/integration/btc-stake.test.js
- script/17_WithdrawalValidator.s.sol
- src/interfaces/IExocoreGateway.sol
- script/12_RedeployClientChainGateway.s.sol
- script/BaseScript.sol
- script/1_Prerequisites.s.sol
- test/foundry/DepositWithdrawPrincipal.t.sol
- test/foundry/Governance.t.sol
- src/lzApp/OAppSenderUpgradeable.sol
- src/core/RewardVault.sol
- script/5_Withdraw.s.sol
- test/foundry/Delegation.t.sol
- test/foundry/unit/UTXOGateway.t.sol
- script/13_DepositValidator.s.sol
- src/interfaces/IClientChainGateway.sol
- script/7_DeployBootstrap.s.sol
- test/foundry/TvlLimits.t.sol
- script/10_DeployExocoreGatewayOnly.s.sol
- src/core/ClientChainGateway.sol
- test/mocks/ExocoreGatewayMock.sol
- script/deployBeaconOracle.s.sol
- test/foundry/DepositThenDelegateTo.t.sol
- script/15_DeploySafeMulstisigWallet.s.sol
- script/14_CorrectBootstrapErrors.s.sol
- slither.config.json
- hardhat.config.js
- test/mocks/DelegationMock.sol
- src/libraries/SignatureVerifier.sol
- test/foundry/ExocoreDeployer.t.sol
- src/core/ExocoreGateway.sol
- test/foundry/unit/ClientChainGateway.t.sol
🧰 Additional context used
🧠 Learnings (6)
script/hardhat/deploy-and-setup-utxogateway.script.js (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: script/hardhat/deploy-and-setup-utxogateway.script.js:84-85
Timestamp: 2025-01-22T06:14:09.961Z
Learning: In the Exocore project, direct contract state queries are preferred over event verification for validating contract operations, as state is the source of truth.
src/interfaces/precompiles/IAssets.sol (2)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/interfaces/precompiles/IAssets.sol:26-45
Timestamp: 2025-01-22T03:46:31.059Z
Learning: Interface files (.sol files containing only interface definitions) should not contain implementation details like validation logic - they should only define the contract structure and function signatures.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/interfaces/precompiles/IAssets.sol:158-162
Timestamp: 2025-01-22T02:55:03.094Z
Learning: Interface files (*.sol files prefixed with 'I') should only contain function declarations without implementation details or validation requirements. Implementation details belong in the concrete contract files.
test/mocks/AssetsMock.sol (3)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#23
File: test/mocks/AssetsMock.sol:9-28
Timestamp: 2024-11-12T10:03:05.536Z
Learning: The `AssetsMock` contract is used for simulating a production precompiled contract, and its complexity in data structures is intentional to mimic production behavior.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/mocks/AssetsMock.sol:149-156
Timestamp: 2025-01-22T03:59:43.806Z
Learning: Mock implementations should strictly follow their interfaces without adding extra parameters or functionality beyond what's defined in the interface.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/mocks/AssetsMock.sol:177-186
Timestamp: 2025-01-22T03:57:41.625Z
Learning: In the `updateAuthorizedGateways` function of the `AssetsMock` contract, duplicate gateway addresses in the input array are acceptable as they don't cause any unexpected effects when setting the same authorization flag multiple times.
src/storage/UTXOGatewayStorage.sol (2)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/storage/UTXOGatewayStorage.sol:457-462
Timestamp: 2025-01-20T02:28:24.440Z
Learning: In the UTXOGateway contract, Token.NONE cannot be registered in outboundRegistry because registration requires a valid ClientChainID and goes through _registerAddress function which has multiple validations. The clientChainId parameter comes from validated sources (stake messages, etc.), making additional Token.NONE checks in the isRegistered modifier redundant.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/storage/UTXOGatewayStorage.sol:457-462
Timestamp: 2025-01-20T02:28:24.440Z
Learning: In the UTXOGateway contract, Token.NONE cannot be registered in outboundRegistry due to validations during the registration process, making additional Token.NONE checks in the isRegistered modifier redundant.
src/core/UTXOGateway.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: src/core/UTXOGateway.sol:407-424
Timestamp: 2025-01-08T03:15:08.317Z
Learning: The `onlyAuthorizedWitness` modifier checking if `msg.sender` is an authorized witness provides sufficient access control for functions that should only be called by witnesses in the UTXOGateway contract.
test/hardhat/integration/btc-stake-e2e.test.js (2)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/hardhat/integration/btc-stake-e2e.test.js:0-0
Timestamp: 2025-01-20T02:24:57.006Z
Learning: In the Exocore project's test suite, prefer using interval-based contract state querying over event listeners for monitoring state changes, as it provides better control and reliability.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#132
File: test/hardhat/integration/btc-stake-e2e.test.js:24-36
Timestamp: 2025-01-08T03:12:49.494Z
Learning: In Bitcoin transaction tests, prefer using test-level timeouts over function-level timeouts to handle long-running operations, as it provides a single point of control and avoids redundant timeout handling.
🪛 LanguageTool
docs/ClientChainGateway.md
[duplication] ~111-~111: Possible typo: you repeated a word.
Context: ...to receive tokens ### NST Restaking 1. Call createExoCapsule 2. Call stake to become validator 3. Call `ve...
(ENGLISH_WORD_REPEAT_RULE)
docs/btc-staking-e2e-testing.md
[uncategorized] ~23-~23: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... send test token to faucet and activate auto mining by running script: ```bash ./start-reg...
(AUTO_HYPHEN)
[uncategorized] ~71-~71: When ‘set-up’ is used as a noun or modifier, it needs to be hyphenated.
Context: ...at.config.js` with the URL path of your set up Exocore node, and also set the private ...
(VERB_NOUN_CONFUSION)
🪛 GitHub Check: lint
src/utils/Dependencies.sol
[failure] 4-4:
imported name CREATE3Factory is not used
🪛 GitHub Actions: Solhint
src/utils/Dependencies.sol
[error] 4-4: imported name CREATE3Factory is not used no-unused-import
🪛 Biome (1.9.4)
test/hardhat/integration/btc-stake-e2e.test.js
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (21)
src/storage/UTXOGatewayStorage.sol (2)
461-466: Confirmed redundancy regardingToken.NONEcheck.
This modifier doesn’t check if the token isToken.NONE, which is correct per prior discussions sinceToken.NONEcannot be registered inoutboundRegistry. No additional checks are needed here.
497-507: Consider upgradable nonce overflow checks.
The_verifyInboundNoncefunction assumes thatinboundNonce[srcChainId] + 1will not overflow. If the system may run long enough for nonce increments to approachtype(uint64).max, consider adding checks or an emergency rollover mechanism.src/core/UTXOGateway.sol (2)
57-73: Protect integrity when removing all witnesses.
Yourinitializefunction ensures at least one witness is provided initially. However, later calls could remove all witnesses if you drop the count from 1 to 0. Currently,_removeWitnessforbids removing the last witness ifauthorizedWitnessCount <= 1, which is good. Validate that you truly never intend to allow zero witnesses.
178-226: Double-check potential for re-entrancy in proof submission.
The code usesnonReentrant, which is good. Ensure that no external calls or hooks can re-invokesubmitProofForStakeMsgin a nested manner, especially before transaction deletion.test/mocks/AssetsMock.sol (4)
12-13: Validate usage ofVIRTUAL_STAKED_BTC_ADDRESS.
Using a placeholder address for BTC staking logic is normal in mocks. Just confirm in integration tests that any address-based checks align with how mainnet or testnet addresses are verified.
37-45: Clarify conditional staker address length for BTC vs. others.
The code enforcesstakerAddress.length == 32only ifclientChainLzId != clientBtcChainId. Confirm that the staker address length for Bitcoin-based usage is handled in a consistent manner across your tests.
149-156: Emptysymbolfield inTokenInfo.
This mock intentionally leavessymbolempty to conform to the tested interface. As previously discussed, it’s a valid approach for a mock.
208-217: Surface differences between unregistered token checks & partial token info.
WhenisRegisteredToken[clientChainId][tokenId]isfalse, you return a defaultTokenInfo. Ensure calling code properly interprets that default struct as “unregistered,” to avoid silent usage of partially filled data.src/interfaces/precompiles/IAssets.sol (4)
10-24: LGTM! Well-structured token info struct.The struct has clear documentation and appropriate field types for storing token metadata.
26-45: LGTM! Well-structured staker balance struct.The struct has clear documentation and appropriate field types for tracking various balance components.
54-56: LGTM! Documentation updates improve clarity.The changes consistently refer to "assets module" instead of "deposit/withdraw module", better reflecting the module's purpose.
Also applies to: 70-72, 86-87, 101-102
158-162: LGTM! Well-documented function additions.The new functions for managing authorized gateways and querying token/staker info have clear documentation and appropriate signatures.
Also applies to: 174-179, 180-189, 190-199
script/hardhat/deploy-and-setup-utxogateway.script.js (1)
75-162: LGTM! Comprehensive deployment and verification.The deployment process is well-structured with proper verification steps and assertions.
test/hardhat/integration/btc-stake-e2e.test.js (4)
1-25: LGTM! Well-structured setup with environment validation.The imports and constants are properly organized with appropriate environment variable validation.
27-55: LGTM! Robust Bitcoin confirmation polling.The function uses interval-based polling with proper error handling, relying on test-level timeout.
🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
147-253: LGTM! Comprehensive staking transaction creation.The function properly handles all aspects of staking transaction creation, including OP_RETURN output for EVM address.
255-378: LGTM! Well-structured test suite with proper verifications.The test suite uses interval-based state querying and appropriate timeouts to verify the staking flow.
src/libraries/Errors.sol (1)
317-401: LGTM! Comprehensive error definitions for UTXOGateway.The errors are well-documented and appropriately categorized with descriptive names and parameters.
.gitmodules (1)
28-30: LGTM! The create3-factory submodule is properly configured.The submodule addition is well-structured and points to the correct repository.
package.json (2)
4-5: LGTM! Description and main entry point updates are appropriate.The description accurately reflects the repository's purpose, and the main entry point change to hardhat.config.js is correct for a Hardhat project.
11-14: LGTM! Scripts are well-defined for deployment and testing.The scripts section includes necessary commands for testing and UTXOGateway deployment.
Description
since we have refactored the original
ExocoreBTCGatewayin https://github.com/ExocoreNetwork/exocore-contracts/tree/btc-bridge and add unit tests to make btc staking work, we should also perform some e2e test on a local exocore node to make sureexocoredworks well withUTXOGatewaycontract.As we need to do integration test with exocore's assets precompiles, and even with other off-chain extensions like bridge, using hardhat test is a better way to work around some of foundry's problem related with custom stateful precompiles
This PR mainly focus on:
UTXOGatewayremappings.txtandhardhat.config.jsto make itSummary by CodeRabbit
New Features
Refactor & Chore
create3-factoryto enhance project capabilities.Documentation
ClientChainGatewayinterface, detailing new functions and workflows.Tests