refactor(contract): IBridgeCallback to IBridgeCallContext #830
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of bridge-related contracts and interfaces, primarily focusing on renaming and restructuring the bridge callback mechanism. The changes involve updating contract names, modifying interface definitions, and replacing the existing Changes
Sequence DiagramsequenceDiagram
participant Sender
participant Bridge
participant BridgeCallContext
Sender->>Bridge: Initiate Bridge Call
Bridge->>BridgeCallContext: onBridgeCall(sender, tokens, amounts)
alt Call Successful
BridgeCallContext-->>Bridge: Process Tokens
else Call Failed
BridgeCallContext->>Bridge: onRevert(error message)
end
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
solidity/contracts/bridge/FxBridgeLogic.sol (1)
597-597: CallingonBridgeCall
Implementation matches the new interface. Consider returning a status or data to confirm the call outcome if needed.solidity/contracts/bridge/IBridgeCallContext.sol (1)
4-14: Renamed interface and newonRevertmethod
The interface rename fromIBridgeCallbacktoIBridgeCallContextis clear. The addition ofonRevert(bytes memory _msg)is a straightforward approach for revert handling. As a minor nitpick, consider renaming_msgto_reasonfor clarity.solidity/contracts/bridge/FxBridgeLogicETH.sol (1)
Line range hint
627-639: LGTM! Bridge callback implementation with suggested improvements.The implementation correctly handles both the new
onRevertandonBridgeCallmethods from theIBridgeCallContextinterface. The control flow is logical, first checkingeventNonceto determine which callback to execute.Consider these improvements:
- Validate and handle the return values from callback methods
- Add specific events for callback results
- Use more specific error messages
if (_input.to.isContract()) { if (_input.eventNonce > 0) { - IBridgeCallContext(_input.to).onRevert(_input.data); + bool success = IBridgeCallContext(_input.to).onRevert(_input.data); + require(success, "Bridge callback onRevert failed"); return; } - IBridgeCallContext(_input.to).onBridgeCall( + bool success = IBridgeCallContext(_input.to).onBridgeCall( _input.sender, _input.refund, _input.tokens, _input.amounts, _input.data, _input.memo ); + require(success, "Bridge callback onBridgeCall failed"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
contract/compile.sh(1 hunks)contract/contract.go(2 hunks)contract/contract_test.go(1 hunks)contract/ibridge_call_context.sol.go(1 hunks)contract/ibridge_callback.sol.go(0 hunks)solidity/contracts/bridge/BridgeCallFee.sol(0 hunks)solidity/contracts/bridge/FxBridgeLogic.sol(2 hunks)solidity/contracts/bridge/FxBridgeLogicETH.sol(2 hunks)solidity/contracts/bridge/IBridgeCallContext.sol(1 hunks)solidity/contracts/test/BridgeCallContextTest.sol(2 hunks)solidity/test/bridge_fee.ts(6 hunks)solidity/test/submit_bridge_call.ts(5 hunks)x/crosschain/keeper/bridge_call_in.go(1 hunks)
💤 Files with no reviewable changes (2)
- solidity/contracts/bridge/BridgeCallFee.sol
- contract/ibridge_callback.sol.go
✅ Files skipped from review due to trivial changes (1)
- contract/ibridge_call_context.sol.go
🔇 Additional comments (34)
solidity/contracts/bridge/FxBridgeLogic.sol (4)
15-15: Renamed import aligns with new interface scheme
This import reflects the updated IBridgeCallContext interface, replacing IBridgeCallback. The naming looks consistent and clear.
589-589: Irrelevant closing brace
No functional or logical changes are introduced here.
591-591: Conditional check for contract
Checking _input.to.isContract() before the callback is a good safeguard against unintended calls to EOA addresses.
593-593: New onRevert callback
Ensure the data being passed here is the intended payload to handle revert logic. Consider clarifying if _input.data includes or excludes revert reason information.
x/crosschain/keeper/bridge_call_in.go (1)
111-111: Refactor to PackOnBridgeCall
The rename from PackBridgeCallback to PackOnBridgeCall aligns with the new interface and naming convention. Verify that any tests have been updated accordingly.
solidity/test/bridge_fee.ts (9)
7-7: Importing BridgeCallContextTest
Import is consistent with the newly introduced contract.
18-18: Declare bridgeCallContextTest
Naming follows the new interface.
94-97: Contract deployment
Deploying BridgeCallContextTest and passing in fxBridge for context is aligned with the updated architecture.
134-134: Fetching contract address
Retrieving the address for subsequent checks is correct.
145-145: Memo references
Embedding bridgeCallContextTest address into memo looks intentional for bridging logic.
148-149: Balance and flag checks
Good approach verifying the token balance and the contract’s internal state (callFlag).
153-153: Final flag verification
Ensuring the callback logic toggles the flag as expected is a solid test.
167-167: Additional balance check
Again verifying the token movement for bridging.
169-169: Repeated call flag verification
Ensuring consistency of the bridging.
solidity/test/submit_bridge_call.ts (8)
7-7: Importing BridgeCallContextTest
The import is aligned with overall refactor to IBridgeCallContext.
18-18: Declare bridgeCallContextTest
References the new test contract that implements the updated interface.
91-94: Instantiating factory for BridgeCallContextTest
Deployment references the correct contract name.
122-122: Updated test description
Renaming suits the updated approach for bridging calls.
126-126: bridgeCallContextAddress
Gathering the deployed address for next steps.
134-138: Calling submitBridgeCall with new references
Ensures bridging logic is tested against bridgeCallContextTest.
150-154: Post-call checks
Validating callFlag and revertFlag to ensure the correct callback workflow.
156-187: Revert testing
These lines confirm that tokens are refunded and onRevert is triggered when the specified condition is met (eventNonce > 0). Good coverage of the new logic branch.
contract/contract.go (2)
73-75: Use consistent naming for ABIs with the new interface.
Renaming bridgeCallbackABI to bridgeCallContextABI aligns with the newly introduced IBridgeCallContext. Ensure references in other files are updated accordingly to avoid confusion or misuse.
139-140: Validate usage of the renamed PackOnBridgeCall.
Renaming from PackBridgeCallback to PackOnBridgeCall is a clear improvement for clarity. Confirm that all callers to this function across the codebase have been updated to the new signature.
contract/contract_test.go (2)
51-51: Confirm that the new expected data matches the updated function signature.
Renaming the function to PackOnBridgeCall may influence the encoded data. Validate that the “expected” string aligns with the new ABI.
57-57: Good consistency with the refactored function name.
Replacing PackBridgeCallback with PackOnBridgeCall ensures the test remains consistent with the updated contract logic.
solidity/contracts/test/BridgeCallContextTest.sol (6)
4-4: Import aligns with the refactored interface name.
Renaming to IBridgeCallContext.sol reflects the updated contract logic.
8-8: Renamed contract inheriting the correct interface.
Switching from BridgeCallbackTest implementing IBridgeCallback to BridgeCallContextTest implementing IBridgeCallContext matches the new nomenclature.
11-11: New revert flag for extended state tracking.
The introduction of revertFlag broadens the contract’s functionality, providing an additional state check for revert callbacks.
16-16: Initialize revertFlag safely.
Default setting to false is clear and supports reliable toggling in the new onRevert method.
19-19: Renaming bridgeCallback to onBridgeCall clarifies semantics.
The method name now better describes the action taken when a bridge call occurs.
30-32: New onRevert method toggles revertFlag.
This addition provides a structured mechanism for handling revert scenarios, enhancing the contract’s extensibility.
contract/compile.sh (1)
34-34: Reflect updated contract list in the compilation process.
Adding IBridgeCallContext ensures the correct ABI and bytecode are generated for the renamed interface, consistent with the refactoring across the codebase.
solidity/contracts/bridge/FxBridgeLogicETH.sol (1)
15-15: LGTM! Import statement updated correctly.
The import statement has been updated to use the new IBridgeCallContext interface, which is consistent with the PR's refactoring objectives.
Summary by CodeRabbit
New Features
IBridgeCallContextwith methods for handling bridge calls and revert scenarios.Bug Fixes
Documentation
Chores
IBridgeCallback.