feat: add onAbort interface #450
Conversation
📝 WalkthroughWalkthroughThe pull request introduces new structures and an interface in the Changes
Assessment against linked issues
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
Documentation and Community
|
![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)
contracts/Revert.sol (3)
32-50: LGTM! Documentation is comprehensive and clear.The NatSpec comments effectively explain the purpose of each field, particularly the rationale for using
bytestype for the sender field to support non-EVM chains.Consider optimizing struct field ordering for gas efficiency.
The struct fields can be reordered to optimize gas costs by grouping similar-sized fields together:
struct AbortContext { - bytes sender; - address asset; - uint256 amount; - bool outgoing; - uint256 chainID; - bytes revertMessage; + uint256 amount; + uint256 chainID; + address asset; + bool outgoing; + bytes sender; + bytes revertMessage; }
60-66: Enhance interface documentation with implementation guidance.While the interface is well-structured, the documentation should be expanded to include:
- Implementation requirements and constraints
- Expected behavior on abort
- Error handling expectations
- Gas considerations
Example documentation improvement:
/// @title Abortable /// @notice Interface for contracts that support abortable calls. +/// @dev Implementers must: +/// - Handle all fields in AbortContext appropriately +/// - Never revert in onAbort implementation +/// - Be mindful of gas consumption as this may be called in cross-chain context +/// - Emit events for important state changes interface Abortable { /// @notice Called when a revertable call is aborted. /// @param abortContext Abort context to pass to onAbort. + /// @dev This function: + /// - Must not revert + /// - Should handle both EVM and non-EVM chain senders + /// - Should validate chainID against supported chains function onAbort(AbortContext calldata abortContext) external; }🧰 Tools
🪛 GitHub Actions: Lint TS/JS/Sol
[error] 66-66: Code formatting issue detected. Line ending style mismatch in file.
66-66: Fix line ending style to match repository standards.The pipeline failure indicates a line ending style mismatch. Ensure consistent line endings throughout the file.
🧰 Tools
🪛 GitHub Actions: Lint TS/JS/Sol
[error] 66-66: Code formatting issue detected. Line ending style mismatch in file.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/Revert.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
contracts/Revert.sol (1)
Pattern contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.
🪛 GitHub Actions: Lint TS/JS/Sol
contracts/Revert.sol
[error] 66-66: Code formatting issue detected. Line ending style mismatch in file.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: slither
- GitHub Check: test
- GitHub Check: generate
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
contracts/Revert.sol (1)
32-66: Verify integration with existing revert mechanism.The new abort mechanism parallels the existing revert functionality. Ensure all implementers of
Revertableare reviewed for potentialAbortableimplementation requirements.✅ Verification successful
Existing implementations already handle revert scenarios securely
The verification shows that current implementers (GatewayEVM, GatewayZEVM, TestUniversalContract) already have robust revert handling with proper security checks. The parallel abort mechanism naturally fits into the existing pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all contracts implementing Revertable interface echo "Contracts implementing Revertable:" ast-grep --pattern 'contract $_ is $$$, Revertable, $$$' # These contracts might need to implement Abortable interface as wellLength of output: 137
Script:
#!/bin/bash # Try different patterns to find Revertable implementations echo "=== Searching for Revertable implementations with different patterns ===" ast-grep --pattern 'contract $_ is Revertable' ast-grep --pattern 'contract $_ is $_Revertable' echo -e "\n=== Searching for onRevert function implementations ===" ast-grep --pattern 'function onRevert(RevertContext $_) $_' echo -e "\n=== Searching for Revertable interface usage ===" rg "Revertable" -A 2Length of output: 18990
🧰 Tools
🪛 GitHub Actions: Lint TS/JS/Sol
[error] 66-66: Code formatting issue detected. Line ending style mismatch in file.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #450 +/- ##
=======================================
Coverage 87.07% 87.07%
=======================================
Files 9 9
Lines 526 526
Branches 128 128
=======================================
Hits 458 458
Misses 68 68 ☔ View full report in Codecov by Sentry. |
| /// token address or is empty if it's a gas token. On ZetaChain, it contains the | ||
| /// address of the ZRC20. | ||
| /// @param amount Amount specified with the transaction. | ||
| /// @param outgoing Flag to indicate if the crosschain transaction was outgoing: from ZetaChain to connected chain. |
There was a problem hiding this comment.
nit: would be wrong to use outbound here as well, or is confusing?
| /// @param outgoing Flag to indicate if the crosschain transaction was outgoing: from ZetaChain to connected chain. | ||
| /// if false, the transaction was incoming: from connected chain to ZetaChain. | ||
| /// @param chainID Chain ID of the connected chain. | ||
| /// @param revertMessage Arbitrary data specified in the revert options |
There was a problem hiding this comment.
can we add more details about revert options?
|
Should we also have chain ID in the revert context? |
Closes: #429
AbortContextstructure is to be discussed, we need context that can address all edge casesSummary by CodeRabbit