refactor(solidity): bridge callback #527
Conversation
WalkthroughThe changes primarily involve renaming the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- contract/IBridgeCallback.go (2 hunks)
- contract/ICrossChain.go (2 hunks)
- contract/IFxBridgeLogic.go (10 hunks)
- contract/compile.sh (1 hunks)
- solidity/contracts/bridge/FxBridgeLogic.sol (12 hunks)
- solidity/contracts/bridge/FxBridgeLogicETH.sol (12 hunks)
- solidity/contracts/bridge/IBridgeCall.sol (1 hunks)
- solidity/contracts/bridge/IBridgeCallback.sol (1 hunks)
- solidity/contracts/bridge/IFxBridgeLogic.sol (4 hunks)
- solidity/contracts/test/BridgeCallbackTest.sol (2 hunks)
- solidity/test/submit_bridge_call.ts (11 hunks)
- x/evm/precompiles/crosschain/bridge_call_test.go (3 hunks)
Files not summarized due to errors (1)
- contract/IFxBridgeLogic.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (2)
- solidity/contracts/bridge/IBridgeCall.sol
- solidity/contracts/test/BridgeCallbackTest.sol
Additional Context Used
Biome (13)
solidity/test/submit_bridge_call.ts (13)
37-113: This function expression can be turned into an arrow function.
164-180: This function expression can be turned into an arrow function.
182-218: This function expression can be turned into an arrow function.
220-244: This function expression can be turned into an arrow function.
252-306: This function expression can be turned into an arrow function.
308-324: This function expression can be turned into an arrow function.
326-346: This function expression can be turned into an arrow function.
348-369: This function expression can be turned into an arrow function.
246-370: This function expression can be turned into an arrow function.
18-371: This function expression can be turned into an arrow function.
1-2: All these imports are only used as types.
3-9: All these imports are only used as types.
27-27: This let declares a variable that is only assigned once.
Additional comments not posted (27)
solidity/contracts/bridge/IBridgeCallback.sol (1)
7-7: The parameter renaming from_receiverto_refundis consistent with the PR objectives and improves clarity.contract/compile.sh (1)
34-34: AddingIBridgeCallbackto the contracts array ensures that the ABI and Go bindings are generated for the updated contract.x/evm/precompiles/crosschain/bridge_call_test.go (1)
23-23: The renaming of the parameter fromReceivertoRefundin the test setup and assertions is correctly implemented and consistent with the changes in the contract interfaces.Also applies to: 33-33, 47-47
solidity/contracts/bridge/IFxBridgeLogic.sol (1)
52-52: The renaming ofreceivertorefundacross struct fields, function parameters, and event parameters is consistent with the PR objectives and correctly implemented.Also applies to: 185-185, 229-229, 242-242
solidity/test/submit_bridge_call.ts (1)
117-117: The renaming of the parameter fromreceivertorefundin the test setup and assertions is correctly implemented and consistent with the changes in the contract interfaces.Also applies to: 129-129, 144-144, 172-172, 233-233, 234-234, 316-316, 338-338, 361-361
contract/IBridgeCallback.go (1)
34-34: The update of the ABI definition and function signatures from_receiverto_refundis consistent with the changes in the Solidity contracts and correctly implemented.Also applies to: 186-186, 193-193, 200-200
solidity/contracts/bridge/FxBridgeLogic.sol (6)
66-66: RenamingreceivertorefundinBridgeCallDatastruct aligns with the PR objectives.
Line range hint
294-305: Ensure that therefundaddress is validated before processing inbridgeCall.
504-504: ThebridgeCallSigHashfunction correctly includes therefundparameter in the data hash. This is a critical change for transaction integrity.
576-598: The_transferAndBridgeCallbackfunction now correctly handles therefundparameter. Ensure that the logic correctly distinguishes betweentoandrefundaddresses based oneventNonce.
457-457: The functionverifySubmitBridgeCallis crucial for security. Ensure that all validations are robust and consider edge cases.Verification successful
The
verifySubmitBridgeCallfunction includes several robust validations:
- Ensures the nonce is unique.
- Validates that the timeout is greater than the current block height.
- Checks that the lengths of tokens and amounts match.
- Verifies the consistency of the oracle set.
- Confirms the validity of the oracle set checkpoint.
These validations appear comprehensive and cover critical security aspects. However, it is always good practice to review these checks periodically and consider any new potential edge cases or vulnerabilities that may arise.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the robustness of validations in `verifySubmitBridgeCall`. # Test: Search for the function usage and review its logic. ast-grep --lang solidity --pattern $'function verifySubmitBridgeCall($_, $_, $_, $_, $_, $_, $_) { $$$ }'Length of output: 225
Script:
#!/bin/bash # Description: Verify the robustness of validations in `verifySubmitBridgeCall`. # Search for the function definition in Solidity files rg 'function verifySubmitBridgeCall' --glob '*.sol' # Search for the function usage across the codebase rg 'verifySubmitBridgeCall' --glob '*.sol'Length of output: 581
Script:
#!/bin/bash # Description: Extract the implementation of `verifySubmitBridgeCall` from both Solidity files for review. # Extract the function implementation from FxBridgeLogic.sol rg -A 20 'function verifySubmitBridgeCall' solidity/contracts/bridge/FxBridgeLogic.sol # Extract the function implementation from FxBridgeLogicETH.sol rg -A 20 'function verifySubmitBridgeCall' solidity/contracts/bridge/FxBridgeLogicETH.solLength of output: 1502
Script:
#!/bin/bash # Description: Extract the complete implementation of `verifySubmitBridgeCall` from both Solidity files for review. # Extract more lines from the function implementation in FxBridgeLogic.sol rg -A 40 'function verifySubmitBridgeCall' solidity/contracts/bridge/FxBridgeLogic.sol # Extract more lines from the function implementation in FxBridgeLogicETH.sol rg -A 40 'function verifySubmitBridgeCall' solidity/contracts/bridge/FxBridgeLogicETH.solLength of output: 2816
Line range hint
814-827: The eventsBridgeCallEventandSubmitBridgeCallEventhave been updated to use the newrefundparameter and_txOrigin. This change should be propagated to all event listeners.solidity/contracts/bridge/FxBridgeLogicETH.sol (8)
77-77: RenamingreceivertorefundinBridgeCallDatastruct aligns with the PR objectives and is consistent with changes inFxBridgeLogic.sol.
Line range hint
330-341: Ensure that therefundaddress is validated before processing inbridgeCall. This is consistent with the changes in the other contract file.
540-540: ThebridgeCallSigHashfunction correctly includes therefundparameter in the data hash. This is a critical change for transaction integrity.
612-634: The_transferAndBridgeCallbackfunction now correctly handles therefundparameter. Ensure that the logic correctly distinguishes betweentoandrefundaddresses based oneventNonce.
351-351: The eventBridgeCallEventnow logs_refundinstead of_receiver. Ensure this change is reflected in all handlers of this event.Verification successful
The search results show that the
BridgeCallEventis indeed being emitted with_refundas a parameter instead of_receiver. This confirms that the change has been made in the event emission.
- Instances of
BridgeCallEventemission:
_msgSender()_refund_toThis indicates that the change from
_receiverto_refundhas been correctly applied in the event emission.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all handlers of `BridgeCallEvent` are updated to use `_refund`. # Test: Search for the event usage. Expect: Only occurances with the new parameter. rg --type solidity $'BridgeCallEvent'Length of output: 69
Script:
#!/bin/bash # Description: Verify all handlers of `BridgeCallEvent` are updated to use `_refund`. # Find all Solidity files and search for the event usage within them. fd -e sol -x rg 'BridgeCallEvent' {}Length of output: 385
Script:
#!/bin/bash # Description: Verify all handlers of `BridgeCallEvent` are updated to use `_refund`. # Find all Solidity files and search for the emission of `BridgeCallEvent` with surrounding lines for context. fd -e sol -x rg -C 3 'emit BridgeCallEvent' {}Length of output: 422
Line range hint
854-867: The eventsBridgeCallEventandSubmitBridgeCallEventhave been updated to use the newrefundparameter and_txOrigin. This change should be propagated to all event listeners.Verification successful
The verification confirms that the
emitstatements forBridgeCallEventandSubmitBridgeCallEventin the filesFxBridgeLogicETH.solandFxBridgeLogic.solhave been updated to include the new parameters_refundand_txOrigin.
BridgeCallEventincludes_msgSender(),_refund,_to, andtx.origin.SubmitBridgeCallEventincludestx.origin,_nonceArray[1],state_lastEventNonce, andsuccess.These changes are consistent across the relevant files.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all listeners of `BridgeCallEvent` and `SubmitBridgeCallEvent` handle the new parameters. # Test: Search for the event usage. Expect: Only occurances with the new parameters. rg --type solidity $'BridgeCallEvent|SubmitBridgeCallEvent'Length of output: 93
Script:
#!/bin/bash # Description: Verify all listeners of `BridgeCallEvent` and `SubmitBridgeCallEvent` handle the new parameters. # Test: Search for the event usage. Expect: Only occurrences with the new parameters. rg 'BridgeCallEvent|SubmitBridgeCallEvent' --glob '*.sol'Length of output: 902
Script:
#!/bin/bash # Description: Verify the parameters used in the `emit` statements for `BridgeCallEvent` and `SubmitBridgeCallEvent`. # Extract lines where `BridgeCallEvent` is emitted rg 'emit BridgeCallEvent' --glob '*.sol' -n # Extract lines where `SubmitBridgeCallEvent` is emitted rg 'emit SubmitBridgeCallEvent' --glob '*.sol' -nLength of output: 435
Script:
#!/bin/bash # Description: Verify the parameters used in the `emit` statements for `BridgeCallEvent` and `SubmitBridgeCallEvent`. # Extract lines around the `emit BridgeCallEvent` statements rg 'emit BridgeCallEvent' --glob '*.sol' -A 5 -B 5 # Extract lines around the `emit SubmitBridgeCallEvent` statements rg 'emit SubmitBridgeCallEvent' --glob '*.sol' -A 5 -B 5Length of output: 3469
493-493: The functionverifySubmitBridgeCallis crucial for security. Ensure that all validations are robust and consider edge cases.
553-553: TheverifySubmitBridgeCallfunction's internal logic appears to be correctly updated to handle the newrefundparameter. Ensure comprehensive testing to avoid security risks.contract/ICrossChain.go (2)
34-34: The ABI update correctly reflects the renaming of_receiverto_refund. Ensure all dependent systems are updated to accommodate this change.
216-218: TheBridgeCallfunction signature has been correctly updated across all bindings to use_refundinstead of_receiver. This change is consistent and well-implemented.Also applies to: 223-225, 230-232
contract/IFxBridgeLogic.go (5)
35-35: Renamed field fromReceivertoRefundinIFxBridgeLogicBridgeCallDatastruct aligns with the PR's objective to standardize terminology.
62-62: Updated ABI inIFxBridgeLogicMetaDatato reflect the renaming of parameters from_receiverto_refund. This change is consistent with the overall goal of the PR.
770-772: ThebridgeCallCheckpointfunction signature has been updated to use_refund. This change is consistent and correctly implemented according to the PR description.Also applies to: 777-779, 784-786
1218-1218: TheBridgeCallEventstruct now uses_refundinstead of_receiver, aligning with the changes made throughout the contract. This is a necessary update to maintain consistency in event parameters.
749-751: ThebridgeCallfunction signature has been correctly updated to use_refundinstead of_receiver. Ensure that all calls to this function across the codebase are updated to match this new signature.Also applies to: 756-758, 763-765
Summary by CodeRabbit
New Features
Refactor
_refundinstead of_receiver.verifyBridgeCalltoverifySubmitBridgeCallto better reflect its purpose.BridgeCallbackTestby removingSafeERC20Upgradeableusage.Tests