feat: handle failed transfers inside onAbort #44
Conversation
📝 WalkthroughWalkthroughThis pull request updates multiple components of the NFT and token contracts along with project configuration files. Changes include adjusting revert option parameters, modifying function visibility, and adding new functions—such as a payable receive function and an onAbort handler—to improve cross-chain transfer error handling. Additionally, new events have been introduced to track aborted transfers. Updates to dependency versions in package files and minor script adjustments are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as Contract
participant G as Gateway
U->>C: transferCrossChain(parameters)
C->>G: Forward transfer request
G-->>C: Error/revert message received
alt Revert Handling
C-->>C: Execute onRevert(context)
else Abort Handling
C-->>C: Execute onAbort(context)
C->>U: Mint asset back to sender
C->>C: Emit TokenTransferAborted event
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 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
|
| context.revertMessage, | ||
| (address, uint256, string, address) | ||
| ); | ||
| _safeMint(sender, tokenId); |
There was a problem hiding this comment.
do we need some validation of sender or other params here?
There was a problem hiding this comment.
I believe ERC-721 already checks if sender is zero:
![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 (2)
contracts/nft/.gitignore (1)
21-22: Validate the inclusion of configuration directories in ignore rules.The addition of
.openzeppelinandtest-ledgerensures that local configuration artifacts and test ledger data are excluded from version control, which aligns with best practices for keeping the repository clean. Please confirm that ignoring.openzeppelindoes not omit essential deployment artifacts, since some projects might track parts of it.contracts/nft/scripts/localnet.sh (1)
7-7: Extended Sleep Duration for Local Network Startup
The change from a 10-second to a 15-second sleep helps ensure that the Hardhat local network has enough time to initialize before subsequent commands are executed. While this works in practice, consider using a readiness check (e.g., polling a specific endpoint or checking port availability) instead of a fixed sleep duration in the future. This would make the startup process more dynamic and potentially reduce unnecessary delays.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
contracts/nft/yarn.lockis excluded by!**/yarn.lock,!**/*.lockcontracts/token/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
contracts/nft/.gitignore(1 hunks)contracts/nft/contracts/evm/UniversalNFTCore.sol(4 hunks)contracts/nft/contracts/shared/UniversalNFTEvents.sol(1 hunks)contracts/nft/contracts/zetachain/UniversalNFTCore.sol(3 hunks)contracts/nft/package.json(2 hunks)contracts/nft/scripts/localnet.sh(1 hunks)contracts/token/contracts/evm/UniversalTokenCore.sol(3 hunks)contracts/token/contracts/shared/UniversalTokenEvents.sol(1 hunks)contracts/token/contracts/zetachain/UniversalTokenCore.sol(3 hunks)contracts/token/package.json(2 hunks)
🔇 Additional comments (20)
contracts/token/package.json (1)
24-24: Dependency updates align with PR objectivesThe upgrades to
@zetachain/localnet(5.0.0-rc1) and@zetachain/protocol-contracts(12.0.0-rc1) support the onAbort functionality required for handling failed transfers, as mentioned in the PR objectives. The re-addition of@typechain/ethers-v5ensures proper TypeScript support for the Ethereum contracts.Also applies to: 31-31, 60-60
contracts/token/contracts/shared/UniversalTokenEvents.sol (1)
15-15: New event added for tracking aborted transfersThis new event
TokenTransferAbortedenables tracking when token transfers are aborted, which directly supports the PR's objective of handling failed transfers insideonAbort. The event includes the sender address and amount, providing essential information for monitoring and debugging purposes.contracts/nft/contracts/shared/UniversalNFTEvents.sol (1)
24-29: New event for tracking aborted NFT transfersThe
TokenTransferAbortedevent is appropriately designed for NFT transfers with parameters for tracking the sender, tokenId, URI, and direction (outgoing). Theoutgoingboolean parameter helps distinguish between NFTs being transferred from ZetaChain to connected chains versus the opposite direction, which aligns with the two scenarios described in the PR objectives.contracts/nft/package.json (1)
34-34: Dependency updates consistent with token packageThe upgrades to
@zetachain/localnet(5.0.0-rc1) and@zetachain/protocol-contracts(12.0.0-rc1) maintain consistency with the token package and support the onAbort functionality needed for handling failed NFT transfers as described in the PR objectives.Also applies to: 65-65
contracts/token/contracts/evm/UniversalTokenCore.sol (3)
130-130: Good update usinguniversalinstead of the zero addressChanging the RevertOptions parameter to use
universalinstead ofaddress(0)provides better semantic clarity and ensures the revert operation is properly directed to the universal contract.
139-139: Good update usinguniversalinstead of the zero addressSimilar to the previous change, using
universalinstead of a zero address in the RevertOptions is a good practice that improves semantics and ensures proper routing of revert operations.
189-189: Good addition of areceivefunctionAdding a
receivefunction allows the contract to accept Ether transfers, which is necessary for the payable functions liketransferCrossChain. Without this function, direct Ether transfers to the contract would fail.contracts/token/contracts/zetachain/UniversalTokenCore.sol (4)
166-168: Good update to the RevertOptions parametersThe change to use
address(this)for the revert destination and including the receiver address in the encoded message provides more context during revert operations, which helps with proper error handling.
235-235: Good update to include the receiver in the encoded messageIncluding the receiver address in the revert message provides more context for the revert operation and aligns with the changes in other parts of the contract.
248-251: Updated decoding to match new message formatThe decoding structure has been correctly updated to match the new message format, which now includes the receiver address as the first parameter.
256-263: Excellent addition of theonAbortfunctionThis new function is a key part of the PR objectives, handling failed transfers by minting tokens back to the original sender on ZetaChain. The implementation correctly decodes the revert message, mints the tokens, and emits an appropriate event.
This function handles the scenario described in the PR where transfers fail due to insufficient tokens and ensures tokens are returned to the sender.
contracts/nft/contracts/zetachain/UniversalNFTCore.sol (5)
172-174: Good update to the RevertOptions parametersUsing
address(this)for the revert destination and including complete context (receiver, tokenId, uri, sender) in the encoded message provides more information during revert operations.
248-248: Good update to include complete context in the encoded messageIncluding the receiver, tokenId, uri, and sender in the revert message provides all necessary information for proper handling during revert operations.
261-264: Updated decoding to match new message formatThe decoding structure has been correctly updated to match the new message format, which now includes the receiver address as the first parameter.
270-287: Excellent addition of theonAbortfunction with detailed documentationThis function is a critical improvement that handles failed transfers by minting NFTs back to the original sender on ZetaChain. The implementation correctly:
- Decodes the revert message to extract tokenId, URI, and sender information
- Safely mints the NFT back to the sender
- Sets the token URI
- Emits a detailed event with the context of the aborted transfer
The comprehensive documentation clearly explains the function's purpose and the scenario it addresses, which enhances code maintainability.
284-284: Consider adding sender validationWhile ERC-721 does check if the receiver address is zero in
_safeMint, you might want to add an explicit check thatsender != address(0)before minting to ensure the token isn't lost if the sender address is somehow invalid.contracts/nft/contracts/evm/UniversalNFTCore.sol (4)
131-131: Good update usinguniversalinstead of the zero addressChanging the RevertOptions parameter to use
universalinstead ofaddress(0)provides better semantic clarity and ensures the revert operation is properly directed to the universal contract.
140-140: Good update usinguniversalinstead of the zero addressSimilar to the previous change, using
universalinstead of a zero address in the RevertOptions is a good practice that improves semantics and ensures proper routing of revert operations.
229-229: Good addition of areceivefunctionAdding a
receivefunction allows the contract to accept Ether transfers, which is necessary for the payable functions liketransferCrossChain. Without this function, direct Ether transfers to the contract would fail.
185-185:❓ Verification inconclusive
Security consideration:
onRevertno longer restricted to gatewayRemoving the
onlyGatewaymodifier means this function can now be called by any address, not just the gateway contract. This could potentially allow unauthorized users to mint tokens by callingonRevertdirectly.Is this intentional? If so, ensure there are other safeguards in place to prevent misuse of this function.
🏁 Script executed:
#!/bin/bash # Check if onRevert is called from anywhere other than the gateway in the codebase echo "Searching for onRevert calls in the codebase..." rg -A 2 -B 2 "onRevert\(" --type solLength of output: 171
Security: Confirm onRevert Access Restrictions
The removal of the
onlyGatewaymodifier incontracts/nft/contracts/evm/UniversalNFTCore.solmeans that theonRevertfunction is now callable by any address, which could enable unauthorized minting unless other safeguards are in place. The initial search for invocations ofonRevertusingrgwith the Solidity file type filter did not yield results due to file type recognition issues.To double-check its usage across the codebase, please run a more inclusive search such as:
#!/bin/bash echo "Searching for onRevert invocations using fd and grep..." fd -e sol | xargs grep -R "onRevert(" -nEnsure that all external calls to
onRevertare intentional and that adequate security measures (e.g., alternative access controls) protect against unauthorized use.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contracts/token/scripts/localnet.sh (1)
7-7: Evaluate the Reduced Sleep DurationThe changed line reduces the fixed waiting period from a longer duration (previously 25 seconds) to 15 seconds after launching the local network. While this speeds up the script execution, please verify that 15 seconds is consistently sufficient for the network to initialize. For increased reliability, consider switching to a dynamic readiness check (e.g., polling via
npx hardhat localnet-checkin a loop) instead of a static sleep.contracts/token/contracts/zetachain/UniversalTokenCore.sol (2)
262-269: Add NatSpec documentation to the onAbort functionThe newly added
onAbortfunction implements the necessary logic for handling aborted transfers, but it's missing NatSpec documentation that explains its purpose and parameters, unlike other functions in the contract.+ /** + * @notice Handles a cross-chain call abortion and returns tokens to the sender. + * @dev This function is called by the Gateway contract when a cross-chain transfer is aborted. + * @param context Metadata about the aborted call. + */ function onAbort(AbortContext calldata context) external onlyGateway {Also, consider whether the
onAbortfunction should handle the case wherecontext.amount > 0 && context.asset != address(0)similar toonRevert, or if there's a specific reason for the different behavior.
248-251: Consider extracting common decoding logicThe decoding logic for
context.revertMessageis duplicated in bothonRevertandonAbort. Consider extracting this into a private helper function to improve maintainability and reduce duplication.+ /** + * @dev Helper function to decode revert message. + * @param revertMessage The encoded revert message. + * @return receiver The address of the intended receiver. + * @return amount The amount of tokens to be returned. + * @return sender The address of the original sender. + */ + function _decodeRevertMessage(bytes memory revertMessage) + private + pure + returns (address receiver, uint256 amount, address sender) + { + return abi.decode( + revertMessage, + (address, uint256, address) + ); + } + function onRevert(RevertContext calldata context) external onlyGateway { - (, uint256 amount, address sender) = abi.decode( - context.revertMessage, - (address, uint256, address) - ); + (, uint256 amount, address sender) = _decodeRevertMessage(context.revertMessage); _mint(sender, amount); emit TokenTransferReverted(sender, amount); if (context.amount > 0 && context.asset != address(0)) { if (!IZRC20(context.asset).transfer(sender, context.amount)) { revert TransferFailed(); } } } function onAbort(AbortContext calldata context) external onlyGateway { - (, uint256 amount, address sender) = abi.decode( - context.revertMessage, - (address, uint256, address) - ); + (, uint256 amount, address sender) = _decodeRevertMessage(context.revertMessage); _mint(sender, amount); emit TokenTransferAborted(sender, amount); }Also applies to: 262-269
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
contracts/nft/yarn.lockis excluded by!**/yarn.lock,!**/*.lockcontracts/token/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
contracts/nft/contracts/evm/UniversalNFTCore.sol(3 hunks)contracts/nft/contracts/zetachain/UniversalNFTCore.sol(4 hunks)contracts/nft/package.json(1 hunks)contracts/token/contracts/evm/UniversalToken.sol(0 hunks)contracts/token/contracts/zetachain/UniversalTokenCore.sol(4 hunks)contracts/token/package.json(1 hunks)contracts/token/scripts/localnet.sh(1 hunks)
💤 Files with no reviewable changes (1)
- contracts/token/contracts/evm/UniversalToken.sol
🚧 Files skipped from review as they are similar to previous changes (4)
- contracts/token/package.json
- contracts/nft/package.json
- contracts/nft/contracts/evm/UniversalNFTCore.sol
- contracts/nft/contracts/zetachain/UniversalNFTCore.sol
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: slither (contracts/token, token.sarif)
- GitHub Check: slither (contracts/nft, nft.sarif)
🔇 Additional comments (3)
contracts/token/contracts/zetachain/UniversalTokenCore.sol (3)
166-167: Enhanced revert handling with proper contextThe updated
RevertOptionsnow usesaddress(this)instead ofaddress(0)for therevertToAddressparameter, ensuring that the revert message is properly routed back to this contract. The encoded parameters now include the receiver address, which provides more comprehensive context for debugging and transaction tracking.
235-235: Consistent revert message patternThe encoded parameters in the
RevertOptionshave been updated to match the structure used intransferCrossChain, which provides consistency in error handling across different functions.
248-251: Updated decoding to match new message formatThe decoding of
context.revertMessagehas been updated to extract the receiver address that was added to the encoded message, ensuring proper handling of reverted transfers.
fadeev
requested review from
skosito and
a team
and removed request for
skosito
|
@s2imonovic please, review. |
| @@ -128,7 +128,7 @@ abstract contract UniversalNFTCore is | |||
| gateway.call( | |||
There was a problem hiding this comment.
whats going on with the native tokens in the case if destination == address(0) and msg.value > 0?
There was a problem hiding this comment.
Good question. Created an issue to handle this in a separate PR.
| @@ -127,7 +127,7 @@ abstract contract UniversalTokenCore is | |||
| gateway.call( | |||
There was a problem hiding this comment.
same question here: whats going on with the native tokens in the case if destination == address(0) and msg.value > 0?
There was a problem hiding this comment.
Will address both questions in a separate PR, see #59
onAborthandles two scenarios:onCallfails, the protocol cannot make a revert,onAbortis called on ZetaChain and the token is transferred to the original sender on ZetaChain.onReverton ZetaChain also fails. Since the logic inonRevertandonAbortis identical right now, this will not do much.Testing with zeta-chain/localnet#103
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Chores
Bug Fixes