Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

Take send cost on BH into account #142

Closed
wants to merge 14 commits into from

Conversation

yrong
Copy link

@yrong yrong commented Apr 16, 2024

Context

This PR tries to fix the issue that send cost on BH is not taken into account, should be included and match the deliveryCost on Ethereum.

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

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

@yrong yrong marked this pull request as ready for review April 16, 2024 14:17
@yrong yrong changed the title Take send cost on BH into account as part of the deliveryCost on Ethereum Take send cost on BH into account Apr 29, 2024
@yrong
Copy link
Author

yrong commented Apr 29, 2024

Add a runtime test calculate the delivery cost:

cargo test -p bridge-hub-rococo-runtime --test snowbridge fetch_delivery_cost -- --nocapture
inbound_delivery_cost:1000092265984
inbound_send_cost:104766618
outbound_delivery_cost:16903333

As result above shows inbound_send_cost consumes a tiny portion of the total(<0.1%) which can be ignored, so this PR seems unnecessary.

Copy link

@vgeddes vgeddes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Ron, excuse the tardy response on this, I was mostly OOTO last week.

Yeah, I was initially aware that XCM sends could have a send cost, but when I looked at the runtime config these costs seemed to be zero, so I avoided having to implement it.

Do you know how these send costs are configured and calculated in the xcm executor?

@vgeddes
Copy link

vgeddes commented Apr 29, 2024

As result above shows inbound_send_cost consumes a tiny portion of the total(<0.1%) which can be ignored, so this PR seems unnecessary.

Lets add a linear ticket for this, so we can track it when scheduling priorities.

@yrong
Copy link
Author

yrong commented May 2, 2024

how these send costs are configured and calculated in the xcm executor?

For sending xcm to sibling chains the cost is calculated with

let price = T::PriceForSiblingDelivery::price_for_delivery(id, &xcm);

Add a ticket for tracing in https://linear.app/snowfork/issue/SNO-980

@yrong yrong closed this May 2, 2024
@vgeddes
Copy link

vgeddes commented May 3, 2024

Reopening this, even though its low priority. Otherwise we may delete the branch by mistake.

@vgeddes vgeddes reopened this May 3, 2024
Copy link

@alistair-singh alistair-singh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

bridges/snowbridge/pallets/inbound-queue/src/lib.rs Outdated Show resolved Hide resolved
@@ -141,6 +141,9 @@ pub mod pallet {

/// To withdraw and deposit an asset.
type AssetTransactor: TransactAsset;

/// The most expensive xcm here only used to estimate send cost
type MaxSendCostXcm: Get<Xcm<()>>;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of configuring an XCM here to set the fee, I would prefer we use a constant fee and then in a test assert that the const equates to the xcm so we know if when the fee diviates.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<Runtime as snowbridge_pallet_inbound_queue::Config>::MaxMessageSize::get(),
);
let inbound_send_cost =
<Runtime as snowbridge_pallet_inbound_queue::Config>::MaxSendCost::get();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably assert that the configured cost is greater than or equal to the xcm cost using the same method you used here:

8ad87ba#diff-832917c2fd6302a9f175210f7d9941ea82a1a7c5a35aa35e662380074d405203L370-L377

Probably not important for this test. But may be very important for the fellowship runtime.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants