Contracts V2 #1300
Contracts V2 #1300
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2 #1300 +/- ##
=====================================
Coverage ? 68.22%
=====================================
Files ? 24
Lines ? 900
Branches ? 132
=====================================
Hits ? 614
Misses ? 270
Partials ? 16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| _ensureOutboundMessagingEnabled(); | ||
|
|
||
| // Wrap provided ether and transfer to AssetHub agent | ||
| address assetHubAgent = Functions.ensureAgent(Constants.ASSET_HUB_AGENT_ID); |
There was a problem hiding this comment.
Since we know agent of asset-hub do exists in production. Is this check nessessary?
There was a problem hiding this comment.
Yeah the check is not necessary, I'll add a TODO.
contracts/src/v2/Calls.sol
Outdated
| bytes memory payload = | ||
| SubstrateTypes.encodePayloadV2(ticket.origin, ticket.assets, ticket.xcm); |
There was a problem hiding this comment.
Cool, A lot more simple now!
With ticket.xcm does that mean for V2 there is no need to construct xcm on chain any more?
UI will build the xcm and it's the responsibility of relayer to validate the xcm and check the relay is profitable or not.
On BH we just decode the xcm with
VersionedXcm::<()>::decode_with_depth_limit(
MAX_XCM_DECODE_DEPTH,
&mut data,
)
and forward it to AH transparently.
There was a problem hiding this comment.
Yeah, exactly.
Though for registration of native tokens, we need to control the XCM and so we need to build it on chain. See more details in 149c3e4.
There was a problem hiding this comment.
Though for registration of native tokens, control the XCM and so we need to build it on chain
Still not sure fully understand why ticket.assets is required here, why not just build the xcm off chain?
@vgeddes I've removed the MessageConverter completely in yrong/polkadot-sdk#3 which I'd assume is not in use any more, correct me if I'm wrong.
There was a problem hiding this comment.
Still not sure fully understand why ticket.assets is required here, why not just build the xcm off chain?
When the message gets to BH, we convert the payload.assets to instructions like ReserveAssetDeposited,etc, and then append the instructions in payload.xcm.
Or in other words, BH builds final XCM using:
finalXCM = concat(
assets_to_xcm(payload.asssets),
UniversalOrigin(Ethereum)
DescendOrigin(payload.origin)
payload.xcm
)
We can't trust the user to build the entire xcm offchain as they could mint any assets they want. Rather, they can only supply a fragment of the xcm.
There was a problem hiding this comment.
We can't trust the user to build the entire xcm offchain as they could mint any assets they want.
Yeah, that make sense.
Instead of convert the assets to xcm, can we just do some basic check on BH to verify the xcm is constructed correctly based upon the assets? e.g.
ensure!(xcm contains DescendOrigin(payload.origin))
ensure!(xcm contains ReserveAssetDeposited(asset)|WithdrawAsset(asset)) // Depends on the transfer type is ENA or PNA
...
So the point here is on chain we do validate rather than construct.
There was a problem hiding this comment.
Btw: for transfer I'm not sure DescendOrigin(userOrigin) make sense. For PNA we need to burn the asset locked in Ethereum sovereign on AH. IIUC that's why we change origin to UniversalOrigin(GlobalConsensus(network)) before WithdrawAsset here.
There was a problem hiding this comment.
So the point here is on chain we do validate rather than construct.
The issue with this approach is that the validation needs to happen on the Ethereum side in order to do this right. The actual locking/unlocking/minting/burning of tokens occurs on the Ethereum side in solidity, which must match the Assets section of the XCM. If the validation happens on BH or in a relayer, and we reject the message, it is too late because we have already changed the state on the Ethereum side. This is why it is better to build the assets section of the XCM on BH to match what was done on the Ethereum side.
Related comment here: yrong/polkadot-sdk#3 (comment)
There was a problem hiding this comment.
it is too late because we have already changed the state on the Ethereum side
Just curious what will happen in this case. Is this something acceptable assuming we will implement error handing mechanism any way? The message with errored xcm will get trapped on BH and we allow end user to edit/repair it?
Something like we allow user to add fee for message pending from Substrate to Ethereum direction, assuming there is no relayer who will pick it up if it's not profitable.
There was a problem hiding this comment.
Just curious what will happen in this case. Is this something acceptable assuming we will implement error handing mechanism any way?
I think if the message is queued and if the message is profitable, than once submitted to BH we should not fail for any reason (such as validating the xcm for example) and we should try to always push the assets to AH where they can get trapped if the user passes invalid xcm. And we should trust that we constrain the origin to stop users from doing things that they cannot within the xcm, instead of validating.
| @@ -59,7 +56,9 @@ contract UpgradeShell is Script { | |||
| assetHubReserveTransferFee: mDot(100), // 0.1 DOT | |||
| exchangeRate: ud60x18(0.0024e18), | |||
| multiplier: ud60x18(1.33e18), | |||
| rescueOperator: 0x4B8a782D4F03ffcB7CE1e95C5cfe5BFCb2C8e967 | |||
| rescueOperator: 0x4B8a782D4F03ffcB7CE1e95C5cfe5BFCb2C8e967, | |||
There was a problem hiding this comment.
Probably don't need this anymore, since we have already dropped the operator on mainnet?
| UpdateChannelParams memory params = abi.decode(data, (UpdateChannelParams)); | ||
| // @dev Get token address by tokenID | ||
| function tokenAddressOf(bytes32 tokenID) external view returns (address) { | ||
| return CallsV1.tokenAddressOf(tokenID); |
There was a problem hiding this comment.
How come this is constrained to v1? Would it not stay the same for v2?
There was a problem hiding this comment.
in certain cases v1 & v2 underlying implementation are the same, so we just use v1 code.
| function _submitOutbound(Ticket memory ticket) internal { | ||
| ChannelID channelID = ticket.dest.into(); | ||
| Channel storage channel = _ensureChannel(channelID); | ||
| uint256 public constant DISPATCH_OVERHEAD_GAS_V2 = 32_000; |
There was a problem hiding this comment.
How do you calculate this? Comment about it might be useful.
| InboundMessageV2 calldata message, | ||
| bytes32[] calldata leafProof, | ||
| Verification.Proof calldata headerProof, | ||
| bytes32 rewardAddress |
There was a problem hiding this comment.
Won't we just just derive the account from the origin on BH?
There was a problem hiding this comment.
It's different. The origin in InboundMessage is derived from account on source chain, while the rewardAddress is account of the relayer for accepting reward.
| } | ||
| } | ||
|
|
||
| function deposit() external payable {} |
| } | ||
|
|
||
| // @dev Set the operating mode of the gateway | ||
| function setOperatingMode(bytes calldata data) external { |
There was a problem hiding this comment.
Both v1 and v2 paths modify the same storage. So essentially v1 and v2 handlers are just two ways to set the bridge operating mode, and they are not ways to disable v1 and v2 separately.
| rescueOperator: address(0), | ||
| foreignTokenDecimals: foreignTokenDecimals, | ||
| maxDestinationFee: maxDestinationFee, | ||
| weth: weth |
There was a problem hiding this comment.
Make weth first item.
| rescueOperator: address(0), | |
| foreignTokenDecimals: foreignTokenDecimals, | |
| maxDestinationFee: maxDestinationFee, | |
| weth: weth | |
| rescueOperator: address(0), | |
| weth: weth, | |
| foreignTokenDecimals: foreignTokenDecimals, | |
| maxDestinationFee: maxDestinationFee |
Commit:
e1a8578
contracts/src/Functions.sol
Outdated
| uint128 amount | ||
| ) internal { | ||
| withdrawNativeToken(executor, agent, weth(), address(this), amount); | ||
| WETH9(payable(weth())).withdraw(amount); |
There was a problem hiding this comment.
This won't work as Gateway is not allowed to receive Ether.
snowbridge/contracts/src/GatewayProxy.sol
Lines 40 to 44 in 4e17456
A potential fix in 9081585
There was a problem hiding this comment.
Good catch! yeah plz merge in
Co-authored-by: Clara van Staden <[email protected]>
Co-authored-by: Alistair Singh <[email protected]>
| } | ||
| } | ||
|
|
||
| function tokenInfo(address token) external view returns (TokenInfo memory) { |
There was a problem hiding this comment.
Bleeding out TokenInfo internal representation struct.
| catch { | ||
| success = false; | ||
| } | ||
| } | ||
|
|
||
| // Calculate a gas refund, capped to protect against huge spikes in `tx.gasprice` | ||
| // that could drain funds unnecessarily. During these spikes, relayers should back off. | ||
| uint256 gasUsed = _transactionBaseGas() + (startGas - gasleft()); | ||
| uint256 gasUsed = v1_transactionBaseGas() + (startGas - gasleft()); | ||
| uint256 refund = gasUsed * Math.min(tx.gasprice, message.maxFeePerGas); | ||
|
|
||
| // Add the reward to the refund amount. If the sum is more than the funds available | ||
| // in the channel agent, then reduce the total amount | ||
| uint256 amount = Math.min(refund + message.reward, address(channel.agent).balance); |
There was a problem hiding this comment.
Reward is paid from the Agent, this is an issue because now that we support ETH in its raw form, we are now using the same account to pay for fees that we are using to lock ETH. Lowering the cost of the bridge, i.e. subsidising relayers messages will drain the agent account of funds locked by users.
We should move the reward+fee to collecting/payout the reward+fee to a different account like the gateway proxy address. And migrate any existing ETH that agents use to the gateway proxy address at time of upgrade to v2.
We are moving away from the channels model where each parachains has a relayer and collects its own fees and handles rewards so I think it is safe to group it all under gateway proxy address.
| require(msg.value <= type(uint128).max, IGatewayV2.ExceededMaximumValue()); | ||
| require(msg.value >= executionFee + relayerFee, IGatewayV2.InsufficientValue()); | ||
| address assetHubAgent = Functions.ensureAgent(Constants.ASSET_HUB_AGENT_ID); | ||
| payable(assetHubAgent).safeNativeTransfer(msg.value); |
There was a problem hiding this comment.
This one line does a lot, it essentially takes the fee, reward and any locked eth and passes it to the asset hub agent. Maybe we should collect fees and reward in a separate account like the gateway proxy contract address and just leave locked funds in the asset hub agent.
| channel.outboundNonce = channel.outboundNonce + 1; | ||
|
|
||
| // Deposit total fee into agent's contract | ||
| payable(channel.agent).safeNativeTransfer(fee); |
There was a problem hiding this comment.
Same here about the fees, we should collect the fees into the gateway contracts asset hub account.
| payable(channel.agent).safeNativeTransfer(fee); | ||
|
|
||
| // Reimburse excess fee payment | ||
| if (msg.value > fee) { |
There was a problem hiding this comment.
Check dust before transfer.
| if (ch.agent == address(0)) { | ||
| revert ChannelDoesNotExist(); | ||
| } | ||
| function v2_isDispatched(uint64 nonce) external view returns (bool) { |
There was a problem hiding this comment.
This method is general enough that we do not have to prefix it with v2_
Tasks:
IGatewayintoIGatewayV1andIGatewayV2