Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1676 +/- ##
==========================================
- Coverage 81.20% 81.04% -0.16%
==========================================
Files 22 22
Lines 1016 997 -19
Branches 196 186 -10
==========================================
- Hits 825 808 -17
+ Misses 174 172 -2
Partials 17 17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Initial plan * Fix dispatchSuccess initialization to false Co-authored-by: yrong <4383920+yrong@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: yrong <4383920+yrong@users.noreply.github.com>
* Initial plan * Remove extra blank line for consistency Co-authored-by: yrong <4383920+yrong@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: yrong <4383920+yrong@users.noreply.github.com>
* Initial plan * Implement Option A: Propagate InsufficientGasLimit from v2_dispatch Co-authored-by: yrong <4383920+yrong@users.noreply.github.com> * Simplify InsufficientGasLimit rethrow logic Co-authored-by: yrong <4383920+yrong@users.noreply.github.com> * Fix tests --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: yrong <4383920+yrong@users.noreply.github.com> Co-authored-by: ron <yrong1997@gmail.com>
…sion (#1690) * Initial plan * Fix: Check atomic flag before emitting CommandFailed event Co-authored-by: yrong <4383920+yrong@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: yrong <4383920+yrong@users.noreply.github.com>
contracts/src/Gateway.sol
Outdated
|
|
||
| // independently, such that failures emit a `CommandFailed` event without stopping execution of | ||
| // subsequent commands. Returns true if all commands executed successfully, false if any command failed. | ||
| function v2_dispatch(InboundMessageV2 calldata message) external onlySelf returns (bool) { |
There was a problem hiding this comment.
The UI can dry-run this method against the entire InboundMessageV2 message, which is especially useful when we extend use cases to messages composed of multiple calls or commands, such as in the L2 integration.
There was a problem hiding this comment.
How can it do though if this function is secured by onlySelf? Can eth_estimateGas simulate a fake origin?
There was a problem hiding this comment.
Correct. Both eth_call and eth_estimateGas can and they do not require signature.
{
"jsonrpc": "2.0",
"method": "eth_estimateGas",
"params": [
{
"from": "0xYourSenderAddressHere",
"to": "0xTargetContractAddress",
"data": "0x..."
}
],
"id": 1
}
E.g. from our api:
snowbridge/web/packages/operations/src/register_erc20.ts
Lines 34 to 40 in 06c4491
claravanstaden
left a comment
There was a problem hiding this comment.
Probably needs a re-audit. I can ask to audit this together with the L2 contracts?
contracts/src/Gateway.sol
Outdated
|
|
||
| return allCommandsSucceeded; | ||
| // Helper function to dispatch a single command with try-catch for error handling | ||
| function v2_dispatchCommand(CommandV2 calldata command, bytes32 origin) external onlySelf { |
There was a problem hiding this comment.
I think we can perform more refactoring to remove all the intermediate internal calls.
v2_dispatchCommand could call HandlersV2.unlockNativeToken directly, and so on?
contracts/src/Gateway.sol
Outdated
| } catch (bytes memory reason) { | ||
| // If insufficient gas limit, rethrow the error to stop processing | ||
| // Otherwise, silently ignore command failures | ||
| if (reason.length >= 4 && bytes4(reason) == IGatewayV2.InsufficientGasLimit.selector) { |
There was a problem hiding this comment.
Not a fan of using Solidity's somewhat awkward exceptions as a control flow mechanism. They are a bit half-baked.
Like in golang, could v2_dispatch return (bool insufficientGas, bool _success)?
contracts/src/Gateway.sol
Outdated
|
|
||
| // independently, such that failures emit a `CommandFailed` event without stopping execution of | ||
| // subsequent commands. Returns true if all commands executed successfully, false if any command failed. | ||
| function v2_dispatch(InboundMessageV2 calldata message) external onlySelf returns (bool) { |
There was a problem hiding this comment.
Its also a bit awkward that InboundMessageV2 is built by BridgeHub, and contains fields such as nonce and origin which are irrelevant for dry-running. It means the UI needs to fake those.
Consider this alternative signature:
function v2_dispatch(Command[] calldata commands)
returns (
bool insufficientGasLimit,
bool failed,
uint256 failureIndex
)Then the UI only needs to format commands. The parent for v2_dispatch can interpret the return values to set the final error state.
There was a problem hiding this comment.
1fa3437#diff-74470ba8f35b42dbc8805739e92708c0696343e0b1819feb3e7a5ebb77cc623c
@vgeddes I made further refactoring based on your suggestion. However, the failureIndex output, as mentioned above, is not suitable, as multiple commands may fail. Additionally, for the input parameters, the nonce is required when emitting IGatewayV2.CommandFailed(nonce, i), and origin is necessary for calling contracts.
The updated dry-run function signature is as follows:
function v2_dispatch(CommandV2[] calldata commands, bytes32 origin, uint64 nonce)
external
onlySelf
returns (bool insufficientGasLimit, bool success)
Let me know if you'd like to make further adjustments.
Context
v2_dispatchfunction to allow the UI to dry-run the entire message instead of each individual command.