Feat/gnosis proxy compiler bump #54
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several new Solidity contracts and an interface for cross-chain arbitration in the Realitio platform. The new contracts Changes
Sequence DiagramsequenceDiagram
participant User
participant HomeProxy
participant ForeignProxy
participant Arbitrator
User->>HomeProxy: Request Arbitration
HomeProxy->>ForeignProxy: Send Arbitration Request
ForeignProxy->>Arbitrator: Create Dispute
Arbitrator->>ForeignProxy: Provide Ruling
ForeignProxy->>HomeProxy: Send Arbitration Answer
HomeProxy->>User: Resolve Dispute
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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: 4
🧹 Nitpick comments (3)
contracts/src/0.8/RealitioHomeProxyGnosis.sol (2)
138-138: Specify a reasonable gas limit instead of usingamb.maxGasPerTx()Using
amb.maxGasPerTx()may lead to inefficient gas usage or potential issues if the maximum gas per transaction changes. It's advisable to set an appropriate gas limit based on the expected computational requirements of the message processing on the foreign proxy.Apply this diff to specify a reasonable gas limit:
bytes4 selector = IForeignArbitrationProxy.receiveArbitrationAcknowledgement.selector; bytes memory data = abi.encodeWithSelector(selector, _questionID, _requester); - amb.requireToPassMessage(foreignProxy, data, amb.maxGasPerTx()); + uint256 gasLimit = 200000; // Set an appropriate gas limit + amb.requireToPassMessage(foreignProxy, data, gasLimit);
163-163: Specify a reasonable gas limit instead of usingamb.maxGasPerTx()Consistent with best practices, specify an appropriate gas limit instead of
amb.maxGasPerTx()to ensure efficient gas usage and prevent potential issues if the max gas per transaction changes.Apply this diff:
bytes4 selector = IForeignArbitrationProxy.receiveArbitrationCancelation.selector; bytes memory data = abi.encodeWithSelector(selector, _questionID, _requester); - amb.requireToPassMessage(foreignProxy, data, amb.maxGasPerTx()); + uint256 gasLimit = 200000; // Set an appropriate gas limit + amb.requireToPassMessage(foreignProxy, data, gasLimit);contracts/src/0.8/RealitioForeignProxyGnosis.sol (1)
412-412: Specify a reasonable gas limit instead of usingamb.maxGasPerTx()Using
amb.maxGasPerTx()might lead to inefficient gas consumption or potential issues if the maximum gas per transaction changes. It's better to set an appropriate gas limit based on the expected computational requirements.Apply this diff to specify an appropriate gas limit:
bytes memory data = abi.encodeWithSelector(methodSelector, bytes32(arbitrationID), bytes32(realitioRuling)); - amb.requireToPassMessage(homeProxy, data, amb.maxGasPerTx()); + uint256 gasLimit = 200000; // Set an appropriate gas limit + amb.requireToPassMessage(homeProxy, data, gasLimit);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/src/0.8/RealitioForeignProxyGnosis.sol(1 hunks)contracts/src/0.8/RealitioForeignProxyOptimism.sol(4 hunks)contracts/src/0.8/RealitioHomeProxyGnosis.sol(1 hunks)contracts/src/0.8/interfaces/IAMB.sol(1 hunks)
🔇 Additional comments (4)
contracts/src/0.8/interfaces/IAMB.sol (1)
1-18: Interface definition looks goodThe
IAMBinterface is well-defined and includes the necessary functions for cross-chain message passing and communication. It accurately reflects the required methods for interaction with the Arbitrary Message Bridge.contracts/src/0.8/RealitioForeignProxyOptimism.sol (3)
93-95: LGTM! Constructor changes improve parameter clarity and gas efficiency.The separation of multiplier parameters and use of immutable storage is a good improvement over using an array.
Also applies to: 103-105, 111-113
151-151: LGTM! Function visibility optimizations.Changing from
publictoexternalis more gas efficient for functions that are only called externally.Also applies to: 201-201
405-407: 🛠️ Refactor suggestionAdd return value check for messenger.sendMessage.
The return value of
messenger.sendMessageshould be checked to ensure the message was sent successfully.Let's verify the return type:
If the function returns a boolean, apply this diff:
bytes memory data = abi.encodeWithSelector(methodSelector, bytes32(arbitrationID), bytes32(realitioRuling)); emit Ruling(IArbitrator(msg.sender), _disputeID, finalRuling); - messenger.sendMessage(homeProxy, data, minGasLimit); + require(messenger.sendMessage(homeProxy, data, minGasLimit), "Message sending failed");
Beside standard compiler changes the other changes are
termsOfServicefrom foreignProxy as not needed. ToS is contained inmetadatavar of homeProxyrelayRulefrom Optimism foreignProxy and inlining it withinrulefunction. Optimism bridging doesn't require a separate function for that unlike zk and Arbitrum where any L1-L2 call requires additional feesSummary by CodeRabbit
New Features
RealitioForeignProxyGnosis: Manages arbitration requests and disputesRealitioHomeProxyGnosis: Handles arbitration processes on side-chainsRealitioForeignProxyOptimism: Updated with refined arbitration logicIAMBinterface for cross-chain messagingImprovements