Handle XCM delivery fees for ExportMessage #999
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #999 +/- ##
==========================================
- Coverage 82.82% 81.13% -1.70%
==========================================
Files 51 52 +1
Lines 2073 2115 +42
Branches 72 72
==========================================
- Hits 1717 1716 -1
- Misses 341 384 +43
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
alistair-singh
force-pushed
the
alistair/fee-manager
branch
from
a3875bf
to
28955c1
Compare
alistair-singh
force-pushed
the
alistair/fee-manager
branch
from
b0d37df
to
64c1e5c
Compare
| >(PhantomData<(TokenLocation, EthereumNetwork, ReceiverAccount, AssetTransactor, OutboundQueue)>); | ||
| /// to Snowbridge and splits it between the origin parachains sovereign and the system | ||
| /// treasury account specified by `ReceiverAmount`. | ||
| pub struct XcmExportFeeToSnowbridge<TokenLocation, EthereumNetwork, AssetTransactor, OutboundQueue>( |
There was a problem hiding this comment.
Think we should call this SnowbridgeXcmFeeHandler
There was a problem hiding this comment.
Changed to XcmExportFeeToSibling
| >(PhantomData<(TokenLocation, EthereumNetwork, ReceiverAccount, AssetTransactor, OutboundQueue)>); | ||
| /// to Snowbridge and splits it between the origin parachains sovereign and the system | ||
| /// treasury account specified by `ReceiverAmount`. | ||
| pub struct XcmExportFeeToSnowbridge<TokenLocation, EthereumNetwork, AssetTransactor, OutboundQueue>( |
There was a problem hiding this comment.
should rename OutboundQueue to FeeProvider, to be more generic.
There was a problem hiding this comment.
Addressed in 50a27fc
| //! | ||
| //! Common traits and types shared by runtimes. | ||
| #![cfg_attr(not(feature = "std"), no_std)] | ||
|
|
||
| use bp_rococo::AccountId; |
There was a problem hiding this comment.
I don't think we need to have a dependency on this crate, which is very specific to ParityBridge. See my other comment below related to this.
There was a problem hiding this comment.
Addressed in 50a27fc
| TokenLocation: Get<MultiLocation>, | ||
| EthereumNetwork: Get<NetworkId>, | ||
| AssetTransactor: TransactAsset, | ||
| OutboundQueue: SendMessageFeeProvider<Balance = bp_rococo::Balance>, |
There was a problem hiding this comment.
Think we should rather directly add Balance as a generic parameter instead of adding a dependency on the bp_rococo crate.
| OutboundQueue: SendMessageFeeProvider<Balance = bp_rococo::Balance>, | |
| Balance: BaseArithmetic + Unsigned + Copy | |
| OutboundQueue: SendMessageFeeProvider<Balance = Balance>, |
There was a problem hiding this comment.
Addressed in 50a27fc
| ); | ||
|
|
||
| impl< | ||
| TokenLocation: Get<MultiLocation>, |
There was a problem hiding this comment.
| TokenLocation: Get<MultiLocation>, | |
| FeeAssetLocation: Get<MultiLocation>, |
There was a problem hiding this comment.
Addressed in 50a27fc
| (snowbridge_export, maybe_para_sovereign, maybe_total_supplied_fee) | ||
| { | ||
| let remote_fee = total_fee.saturating_sub(OutboundQueue::local_fee()); | ||
| if remote_fee > 0 { |
There was a problem hiding this comment.
According to my reading, If remote_fee is zero (not likely to happen in practice), the conditional code block is skipped, and the local component of the fees is not deposited into the treasury?
There was a problem hiding this comment.
The local component is sent to the next fee handler.
| if let (true, Some(para_sovereign), Some((fee_index, total_fee))) = | ||
| (snowbridge_export, maybe_para_sovereign, maybe_total_supplied_fee) | ||
| { | ||
| let remote_fee = total_fee.saturating_sub(OutboundQueue::local_fee()); |
There was a problem hiding this comment.
I may be missing something, but it seems instead of substracting, you may want to split the total fee into local and remote components, and handle them separately?
I'm also not seeing the local component of the fees being deposited into the treasury.
There was a problem hiding this comment.
The local component is sent to the next fee handler. We only syphon off the remote fee and explicitly send it to the sibling sovereign.
| /// to Snowbridge and splits off the local remote fee and deposits it to the origin | ||
| /// parachain sovereign account. The local fee is then returned back to be handled by | ||
| /// the next fee handler in the chain. Most likely the treasury account. | ||
| pub struct XcmExportFeeToSnowbridge<TokenLocation, EthereumNetwork, AssetTransactor, OutboundQueue>( |
There was a problem hiding this comment.
Just a general question - How is this XcmExportFeeToSnowbridge type intended to be wired up in the runtime, assuming ParityBridge also has a fee handler which it needs to install.
I'm assuming there will be some kind of tuple or something:
type FeeManager = (XcmExportFeeToParityBridge, XcmExportFeeToSnowbridge)I guess I'm just worried that the two fee handlers will conflict in some manner.
There was a problem hiding this comment.
Actually it's type FeeManager = (XcmExportFeeToSnowbridge,XcmExportFeeToParityBridge)
The HandleFee impl for Tuple as following
https://github.com/Snowfork/polkadot-sdk/blob/f56aaf93e36117392e61e48a59e66ae764d697ff/polkadot/xcm/xcm-builder/src/fee_handling.rs#L40-L56
so remote fee to assethub sovereign and the leftover(i.e. local fee) to treasury.
| ); | ||
|
|
||
| // Get the parachain sovereign from the `context`. | ||
| let maybe_para_sovereign = if let Some(XcmContext { |
There was a problem hiding this comment.
if XcmContext is None, we should perhaps exit early, and allow the next fee handler to process the fee. This may make the code look a bit cleaner.
There was a problem hiding this comment.
Addressed in e0d356f
vgeddes
changed the title
Resolves: SNO-669
Polkadot-Sdk companion: Snowfork/polkadot-sdk#23