Snowbridge: Unpaid execution when bridging to Ethereum

[yrong]

Context

Currently we use SovereignPaidRemoteExporter, which is unnecessary since all fees in Snowbridge V2 will be estimated on the fly and injected into the XCM.

Resolves: https://linear.app/snowfork/issue/SNO-1510

[yrong]

Hey @acatangiu, I've opened this PR to switch to the UnpaidExporter as you mentioned earlier.

I have a quick question: why do we enforce no payment here for UnpaidRemoteExporter? Could we remove this line and attach a fee, similar to what SovereignPaidRemoteExporter does?

I'm asking because, IMHO, the term unpaid refers to execution cost on the destination chain, not the source.

[acatangiu]

I have a quick question: why do we enforce no payment here for UnpaidRemoteExporter? Could we remove this line and attach a fee, similar to what SovereignPaidRemoteExporter does?
I'm asking because, IMHO, the term unpaid refers to execution cost on the destination chain, not the source.

Sounds sane to me. @bkontur any reasons we should not do that?

I am thinking we can check WaivedLocations and simply waive transport fees accordingly based on origin rather than assume/assert they are waived for any origin using this exporter.

[bkontur]

I have a quick question: why do we enforce no payment here for UnpaidRemoteExporter? Could we remove this line and attach a fee, similar to what SovereignPaidRemoteExporter does?
I'm asking because, IMHO, the term unpaid refers to execution cost on the destination chain, not the source.

Sounds sane to me. @bkontur any reasons we should not do that?

I am thinking we can check WaivedLocations and simply waive transport fees accordingly based on origin rather than assume/assert they are waived for any origin using this exporter.

Hmm, I think it make senses. I am checking trait ExporterFor docs:

polkadot-sdk/polkadot/xcm/xcm-builder/src/universal_exports.rs

Lines 159 to 162 in ff5c67c

	/// The payment is specified from the local context, not the bridge chain. This is the
	/// total amount to withdraw in to Holding and should cover both payment for the execution on
	/// the bridge chain and payment for the use of the `ExportMessage` instruction.
	fn exporter_for(

iiuc, the ExporterFor returns the cost, which should cover:

1. remote-execution on the bridged chain
2. delivery fees for ExportMessage

So, if we say that UnpaidRemoteExporter means that with UnpaidExecution we don't need to pay "2. delivery fees for ExportMessage", than yes, the assert for maybe_payment could be removed and the maybe_payment could be added to the cost like @yrong did here: https://github.com/paritytech/polkadot-sdk/pull/8599/files#diff-fb508a860be147ec684bacf49b9432885463edaba1fab99a8fbfc597f98a0685R61-R70.
Anyway this cost will contain XCMP delivery for example.

If we want total unpaid, means not to pay also "1. remote-execution on the bridged chain", then we can set up ExporterFor to return None, so we should be able to cover all the cases.

Also I think we don't need to check WaivedLocation here, because it will be anyway checked with fn take_fee or fn charge_fees -> but definitely we can add a test for this, that WaivedLocations works with UnpaidRemoteExporter with maybe_payment=Some(asset).

I think let's go with changes above for UnpaidRemoteExporter instead of a new SnowbridgeUnpaidRemoteExporter

[acatangiu]

Also I think we don't need to check WaivedLocation here, because it will be anyway checked with fn take_fee or fn charge_fees

yes, I said it wrong, not meant explicitly check, but rather rely on WaivedLocations - proposing the exporter handle delivery fees as normal - the "Unpaid" part is related to the fact that it is using explicit UnpaidExecution instruction

I think let's go with changes above for UnpaidRemoteExporter instead of a new SnowbridgeUnpaidRemoteExporter

yes, @yrong please add delivery fees to UnpaidRemoteExporter rather than adding new SnowbridgeUnpaidRemoteExporter

[yrong]

Since UnpaidRemoteExporter now supports attaching a fee, how about applying it to the Snowbridge V1 route as well?

That would eliminate the need to maintain a balance for AH’s sovereign account on BH.

@vgeddes @acatangiu WDYT?

[acatangiu]

how long do you plan to support Snowbridge v1 for? might not be worth the change if its lifetime is limited anyway (lower risks of breaking something)

[yrong]

As we discussed internally, it’s likely that we will run V1 along side with V2 for quite some time.

Btw, aside from changing this line

polkadot-sdk/bridges/snowbridge/primitives/outbound-queue/src/v1/converter/mod.rs

Line 135 in e723cfa

let fee = Asset::from((Location::parent(), fee.total())).into();

to set the fee to zero, I don’t see many changes required.

There might be some cleanup needed, but considering that our integration tests cover most cases, I’m not too concerned about the breaking things.

[yrong]

A seperate PR for this: yrong#20

[vgeddes]

Looks good! Thanks Ron.

[yrong]

@acatangiu @bkontur I've merged yrong#20. Please take another look at your convenience.

[acatangiu]

Summary [ 244.619s] 3465 tests run: 3461 passed (19 slow, 1 leaky), 4 failed, 7000 skipped
TRY 6 FAIL [ 0.136s] bridge-hub-rococo-runtime::snowbridge transfer_token_to_ethereum_fee_not_enough
TRY 6 FAIL [ 0.083s] bridge-hub-westend-runtime::snowbridge transfer_token_to_ethereum_fee_not_enough
TRY 6 FAIL [ 0.028s] bridge-hub-westend-runtime::snowbridge unpaid_transfer_token_to_ethereum_fails_with_barrier
TRY 6 FAIL [ 0.058s] bridge-hub-westend-runtime::tests governance_authorize_upgrade_works

[Polkadot-Forum]

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/snowbridge-incident-report-polkadot-runtime-upgrade-1-7-1/15144/1