Working version of making acks optional #202
Conversation
WalkthroughThe changes introduced in this pull request enhance the acknowledgment handling within the Inter-Blockchain Communication (IBC) framework. The modifications include the introduction of an Changes
Possibly related PRs
Suggested labels
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (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: 0
Outside diff range and nitpick comments (1)
contracts/libs/Ibc.sol (1)
254-255: Consider handling theAckStatus.SKIPcase.The changes to the
parseAckDatafunction are consistent with the introduction of theAckStatusenum. The function correctly maps the acknowledgment data to the appropriateAckStatusvalue.However, the function does not handle the
AckStatus.SKIPcase. Is this intentional? If skipping is applicable for this specific acknowledgment data, please consider adding a condition to returnAckPacket(AckStatus.SKIP, ...)when appropriate.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- contracts/core/Dispatcher.sol (2 hunks)
- contracts/examples/Earth.sol (2 hunks)
- contracts/examples/Mars.sol (6 hunks)
- contracts/examples/Pluto.sol (1 hunks)
- contracts/libs/Ibc.sol (2 hunks)
- test/Dispatcher/Dispatcher.ack.sol (1 hunks)
- test/Dispatcher/Dispatcher.dappHandlerRevert.t.sol (3 hunks)
- test/Dispatcher/Dispatcher.proof.t.sol (1 hunks)
- test/Dispatcher/Dispatcher.t.sol (3 hunks)
- test/Ibc.t.sol (1 hunks)
- test/mocks/GasUsingMars.sol (2 hunks)
- test/payload/ack.t.sol (2 hunks)
- test/utils/Dispatcher.base.t.sol (1 hunks)
Additional comments not posted (33)
test/mocks/GasUsingMars.sol (2)
6-6: LGTM!Importing
AckStatusis a good practice as it provides a more explicit and semantically meaningful representation of the acknowledgment status.
39-39: LGTM!Using
AckStatus.SUCCESSinstead of a boolean value (true) is a good practice as it provides a more explicit and semantically meaningful representation of the acknowledgment status. This change enhances the clarity and intent of the code by using a defined status rather than a generic boolean.contracts/examples/Pluto.sol (1)
31-51: LGTM!The
Plutocontract is a simple and clear example of how to implement an IBC receiver contract that receives packets without sending acks. The contract is well-documented and follows best practices.Some key points:
- The contract inherits from
Marsand takes anIbcDispatcherin the constructor, which is a good practice for reusability and modularity.- The
onRecvPacketfunction is well-documented with clear instructions on how to validate the incoming packet.- The function appends the received packet to a list, which can be useful for testing and debugging.
- The function returns an acknowledgment packet with a status of
SKIP, which is a valid status for IBC packets.- The acknowledgment packet includes a JSON-encoded message, which is a good practice for interoperability.
- The function is marked with the
onlyIbcDispatchermodifier, which is a good practice for access control.Overall, this contract serves as a good example for dapp developers on how to integrate with the vibc protocol.
test/payload/ack.t.sol (2)
25-25: Looks good!The update to
ackPacket1instantiation aligns perfectly with the changes made in thegetAckPacketBytesfunction. It ensures that the test accurately reflects the intended functionality ofAckPacket.
11-11: LGTM! Verify the usage ofgetAckPacketBytesin the codebase.The change from using a boolean
ack.successto an enumeratedAckStatus.SUCCESSenhances clarity and maintainability. It aligns well with the PR objective of making acks optional.Please ensure that all calls to
getAckPacketByteshave been updated to pass the correctAckStatusvalue. You can use the following script to verify the usage:Verification successful
Verification successful:
getAckPacketByteschange correctly implementedThe modification to
getAckPacketBytesusingAckStatus.SUCCESSinstead of a boolean value has been correctly implemented. The function is only used in test files and not in the main contract code. Furthermore, theAckStatusenum is consistently used throughout the codebase, including in test files, mock contracts, and main contract files.Key observations:
getAckPacketBytesimplementation in test/payload/ack.t.sol is correct.- No instances of
getAckPacketBytesbeing called with a boolean argument were found.AckStatusis widely and consistently used across the codebase (SUCCESS, FAILURE, and SKIP statuses).This change aligns well with the PR objective of making acks optional and enhances the overall clarity and maintainability of the code.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `getAckPacketBytes` pass the correct `AckStatus` value. # Test: Search for the function usage. Expect: Only occurrences with `AckStatus` argument. rg --type solidity -A 5 $'getAckPacketBytes'Length of output: 76
Script:
#!/bin/bash # Description: Verify the usage of getAckPacketBytes and related concepts # Search for getAckPacketBytes usage echo "=== getAckPacketBytes usage ===" rg "getAckPacketBytes" -n # Search for getAckPacketBytes function definition echo -e "\n=== getAckPacketBytes function definition ===" rg "function getAckPacketBytes" -n -A 5 # Search for AckStatus usage echo -e "\n=== AckStatus usage ===" rg "AckStatus" -nLength of output: 4914
test/Dispatcher/Dispatcher.ack.sol (3)
23-23: LGTM!The code segment correctly handles the new return type of the
ackPacketsfunction, which now returns anAckStatusenum instead of a boolean. This change enhances the granularity of the acknowledgment handling by allowing for more detailed status checks.
25-27: LGTM!The code segment correctly compares the integer representation of the
AckStatusenum returned byackPacketswith the integer representation of the status from the parsedAckPacket. This ensures that the status returned byackPacketsmatches the status in the parsedAckPacket.
Line range hint
1-88: No further comments.The rest of the file looks good. The test functions provide good coverage of different scenarios related to acknowledgment handling, and there are no apparent issues or improvements needed.
test/Ibc.t.sol (2)
55-57: LGTM!The code segment correctly compares the expected
AckStatus.SUCCESSenum value with the parsed status from the acknowledgment packet by converting both to their correspondinguint8values.
62-64: LGTM!Similar to the previous code segment, this code segment correctly compares the expected
AckStatus.FAILUREenum value with the parsed status from the error acknowledgment packet by converting both to their correspondinguint8values.test/Dispatcher/Dispatcher.dappHandlerRevert.t.sol (4)
68-68: LGTM!The change from using a boolean value to the
AckStatus.FAILUREenum value improves clarity and type safety when emitting theWriteAckPacketevent.
79-79: LGTM!Similar to the previous change, using the
AckStatus.FAILUREenum value instead of a boolean improves clarity and type safety when emitting theWriteAckPacketevent.
86-86: LGTM!The change to use the
AckStatus.FAILUREenum value in theAckPacketstruct when emitting theWriteAckPacketevent is consistent with the previous changes and improves clarity and type safety.
96-96: LGTM!Once again, using the
AckStatus.FAILUREenum value in theAckPacketstruct when emitting theWriteAckPacketevent is a consistent and clear way to indicate a failure scenario.contracts/examples/Earth.sol (2)
20-20: LGTM!The import statement is syntactically correct and the
AckStatusenum is likely used in the contract to represent the status of an acknowledgment packet.
169-169: Excellent change!Replacing the boolean value with the
AckStatus.SUCCESSenum value enhances the clarity and robustness of the acknowledgment mechanism. It provides a more structured and explicit way to represent the status of an acknowledgment packet.The change is consistent with the import of the
AckStatusenum and improves the overall code quality.test/Dispatcher/Dispatcher.proof.t.sol (1)
131-131: LGTM!The change from using a boolean value to
AckStatus.SUCCESSfor instantiating theAckPacketimproves the clarity and maintainability of the code. It provides a more descriptive and type-safe representation of the acknowledgment status.The change is consistent with the emit statement and aligns the test with the expected behavior of the
recvPacketfunction.contracts/libs/Ibc.sol (2)
68-72: LGTM!The new
AckStatusenum provides more granular and expressive acknowledgment status compared to the previous boolean field. The enum values are well-named and self-explanatory.
77-77: Verify the impact of the field change.The change from
successtostatusis consistent with the introduction of theAckStatusenum. However, please ensure that:
- The
parseAckDatafunction is updated to use the newstatusfield.- Existing code that uses the
successfield ofAckPacketis updated to use thestatusfield.Run the following script to verify the usage of the
successfield:test/utils/Dispatcher.base.t.sol (2)
253-255: Refactor to useAckStatusenumeration improves code clarity and robustness.The change from using a boolean to an enumeration for determining the success of the acknowledgment is a good refactor. It enhances code clarity and robustness by explicitly defining the acknowledgment states.
Ensure that the
AckStatusenumeration is defined and imported correctly.
260-260: Consistent usage ofAckStatus.SUCCESSmaintains code consistency.The change to use
AckStatus.SUCCESSwhen creating theAckPacketis consistent with the refactor in theackToBytesfunction. It maintains code consistency and aligns with the usage of theAckStatusenumeration.contracts/examples/Mars.sol (5)
20-20: LGTM!The import statement for
AckStatusis necessary to support the transition from using boolean values to theAckStatusenum for representing acknowledgment statuses in the subsequent code changes.
90-90: Looks good!Using
AckStatus.SUCCESSinstead oftruefor a successful acknowledgment improves code clarity and aligns with the transition to using theAckStatusenum for representing acknowledgment statuses.
249-249: LGTM!Using
AckStatus.FAILUREinstead offalsefor a failed acknowledgment improves code clarity and aligns with the transition to using theAckStatusenum for representing acknowledgment statuses.
284-284: Looks good!Using
AckStatus.FAILUREinstead offalsefor a failed acknowledgment improves code clarity and aligns with the transition to using theAckStatusenum for representing acknowledgment statuses.
300-300: LGTM!Using
AckStatus.FAILUREinstead offalsefor failed acknowledgments in theRevertingEmptyMarsandPanickingMarscontracts improves code clarity and aligns with the transition to using theAckStatusenum for representing acknowledgment statuses.Also applies to: 309-309
test/Dispatcher/Dispatcher.t.sol (3)
4-4: LGTM!The import statement for the
AckStatusenum is correct and relevant to the test suite.
354-354: Improved clarity in the WriteAckPacket emission.Using the
AckStatus.SUCCESSenum value instead of a boolean enhances the readability and maintainability of the code.
359-368: Valid addition of the test_pluto_noAck test function.The new test function correctly verifies that receiving a packet from the
Plutocontract does not trigger an acknowledgment. It sets up the necessary components, such as creating an instance ofPlutoand constructing anIbcEndpoint, and uses thevm.expectEmitcheatcode to validate the emitted events.contracts/core/Dispatcher.sol (4)
28-28: LGTM!The import statement for
AckStatusis syntactically correct and consistent with the list of alterations.
626-628: Verify the intended behavior and impact of skipping packet processing for theAckStatus.SKIPstatus.The added check for
AckStatus.SKIPintroduces an early return, effectively ignoring the packet when this status is received. Please ensure that this is the intended behavior and consider the potential impact on the packet processing flow.
630-630: LGTM!Using the
AckStatus.FAILUREenum value for the failure case enhances clarity regarding the acknowledgment's outcome compared to the previous boolean representation.
626-630: LGTM!The changes introduce explicit handling of different acknowledgment statuses, improving the robustness of the packet handling logic. Skipping packet processing for
AckStatus.SKIPand setting the status toAckStatus.FAILUREin the failure case align with the expected behavior based on the status values.
| @@ -623,8 +623,11 @@ contract Dispatcher is Ownable2StepUpgradeable, UUPSUpgradeable, ReentrancyGuard | |||
| _callIfContract(receiver, abi.encodeWithSelector(IbcPacketReceiver.onRecvPacket.selector, packet)); | |||
| if (success) { | |||
| (ack) = abi.decode(data, (AckPacket)); | |||
| if (ack.status == AckStatus.SKIP) { | |||
There was a problem hiding this comment.
consider adding some in-line comments explaining that this is the case where app choses not to have an ack
| IbcEndpoint memory plutoDest = IbcEndpoint(plutoPort, channelId); | ||
| vm.expectEmit(true, true, true, true, address(dispatcherProxy)); | ||
| emit RecvPacket(address(pluto), channelId, packetSeq); | ||
| dispatcherProxy.recvPacket(IbcPacket(src, plutoDest, packetSeq, payload, ZERO_HEIGHT, maxTimeout), validProof); |
There was a problem hiding this comment.
in addition to the asserting that the recvPacket event was emitted, we also need to test that:
- the
WriteAckPacketwas NOT emitted _ackPacketCommitmentwas NOT set to true
for the first case, we have to be a bit clever, use expectEmit and emit a unique event after the call, such that the expect emit will pass iff writeAckPacket event was not emitted (I'd suggest reading expect emit docs first to understand how event testing works, it's a tad unintuitive)
for second case, you can use something like findAckPacketCommitmentSlot in test/upgradeableProxy/UpgradeUtils.t.sol to calculate and read from a storage slot in dispatcher proxy, and ensure that that is indeed not set to true
| struct AckPacket { | ||
| // success indicates the dApp-level logic. Even when a dApp fails to process a packet per its dApp logic, the | ||
| // delivery of packet and ack packet are still considered successful. | ||
| bool success; | ||
| AckStatus status; |
There was a problem hiding this comment.
I'm just now realizing that, because this AckPacket type is used in the acknowledgement proof, we can't change this existing type and need to add a new type with the enum. Otherwise we might have to make changes to peptide to support this new enum change.
So I think we need to have two seperate AckPackets types now:
- an
AckPacketwith the enum status which the dapp returns in the onrecv callback - the original
AckPackettype with the bool success that is emitted in the dispatcher events (and in turn used in peptide proofs)
| ? AckPacket(true, Base64.decode(string(ack[11:ack.length - 2]))) // result success | ||
| : AckPacket(false, ack[10:ack.length - 2]); // this is an error | ||
| ? AckPacket(AckStatus.SUCCESS, Base64.decode(string(ack[11:ack.length - 2]))) // result success | ||
| : AckPacket(AckStatus.FAILURE, ack[10:ack.length - 2]); // this is an error |
There was a problem hiding this comment.
See the other comment, but we actually can't change the AckPacket return type here, since parseAckData is used to find the proof key in the acknowledge method in dispatcher. We need to have two seperate ackPacket types now
Summary by CodeRabbit
New Features
AckStatusenumeration for improved acknowledgment handling.Plutocontract to demonstrate IBC functionality.Bug Fixes
SKIP,SUCCESS,FAILURE) more robustly.Tests
AckStatusenumeration instead of boolean values.Plutocontract to ensure correct acknowledgment behavior.