Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a linear fee multiplier (#127) #3790

Merged
merged 4 commits into from
Mar 22, 2024

Conversation

vgeddes
Copy link
Contributor

@vgeddes vgeddes commented Mar 22, 2024

Bridging fees are calculated using a static ETH/DOT exchange rate that can deviate significantly from the real-world exchange rate. We therefore need to add a safety margin to the fee so that users almost aways cover the cost of relaying.

FAQ

Why introduce a multiplier parameter instead of configuring an exchange rate which already has a safety factor applied?

When converting from ETH to DOT, we need to divide the multiplier by the exchange rate, and to convert from DOT to ETH we need to multiply the multiplier by the exchange rate.

Other input parameters to the fee calculation can also deviate from real-world values. These include substrate weights, gas prices, and so on. Why does the multiplier introduced here not adjust those?

A single scalar multiplier won't be able to accommodate the different volatilities efficiently. For example, gas prices are much more volatile than exchange rates, and substrate weights hardly ever change.

So the pricing config relating to weights and gas prices should already have some appropriate safety margin pre-applied.

Detailed Changes:

  • Added multiplier field to PricingParameters
  • Outbound-queue fee is multiplied by multiplier
  • This multiplier is synced to the Ethereum side
  • Improved Runtime API for calculating outbound-queue fees. This API makes it much easier to for configure parts of the system in preparation for launch.
  • Improve and clarify code documentation

Upstreamed from Snowfork#127

* Add fee multiplier

* fmt

* fmt

* Update test mock

* Swap order for parameters to calculate_fee

---------

Co-authored-by: claravanstaden <Cats 4 life!>
@paritytech-review-bot paritytech-review-bot bot requested a review from a team March 22, 2024 09:40
Copy link
Contributor

@yrong yrong left a comment

Choose a reason for hiding this comment

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

+1

@bkontur bkontur added R0-silent Changes should not be mentioned in any release notes T15-bridges This PR/Issue is related to bridges. labels Mar 22, 2024
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

Review of "just the code diff" looks ok, relying on Snowfork expertise for the correctness of its behavior and/or subtle changes.

Comment on lines 65 to 66
//! RemoteFeeAdjusted(Message) = Params.Multiplier * (RemoteFee(Message) / Params.Ratio("ETH/DOT"))
//! Fee(Message) = LocalFee(Message) + Params.Multiplier * RemoteFeeAdjusted(Message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//! RemoteFeeAdjusted(Message) = Params.Multiplier * (RemoteFee(Message) / Params.Ratio("ETH/DOT"))
//! Fee(Message) = LocalFee(Message) + Params.Multiplier * RemoteFeeAdjusted(Message)
//! RemoteFeeAdjusted(Message) = Params.Multiplier * (RemoteFee(Message) / Params.Ratio("ETH/DOT"))
//! Fee(Message) = LocalFee(Message) + Params.Multiplier * RemoteFeeAdjusted(Message)

Looks like multiplying by Params.Multiplier happens twice for final Fee(Message) value? (multiplier is squared)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. That was obsolete documentation. The multiplier is applied only once.

Fixed in 942bfb1

bridges/snowbridge/pallets/outbound-queue/src/lib.rs Outdated Show resolved Hide resolved
acatangiu pushed a commit that referenced this pull request Mar 22, 2024
…nsure safety margins (#3791)

This is a cherry-pick from master of
#3727 and
#3790

Expected patches for (1.7.0):
snowbridge-pallet-ethereum-client
snowbridge-pallet-inbound-queue
snowbridge-pallet-outbound-queue
snowbridge-outbound-queue-runtime-api
snowbridge-pallet-system
snowbridge-core
@bkontur bkontur added this pull request to the merge queue Mar 22, 2024
Merged via the queue into paritytech:master with commit 22d5b80 Mar 22, 2024
127 of 132 checks passed
acatangiu added a commit that referenced this pull request Mar 22, 2024
)

This is a cherry-pick from master of
#3790

Expected patches for (1.7.0):
snowbridge-pallet-ethereum-client
snowbridge-pallet-inbound-queue
snowbridge-pallet-outbound-queue
snowbridge-outbound-queue-runtime-api
snowbridge-pallet-system
snowbridge-core

---------

Co-authored-by: Vincent Geddes <vincent@snowfork.com>
Co-authored-by: claravanstaden <Cats 4 life!>
Co-authored-by: Vincent Geddes <vincent.geddes@hey.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Mar 24, 2024
Bridging fees are calculated using a static ETH/DOT exchange rate that
can deviate significantly from the real-world exchange rate. We
therefore need to add a safety margin to the fee so that users almost
aways cover the cost of relaying.

# FAQ

> Why introduce a `multiplier` parameter instead of configuring an
exchange rate which already has a safety factor applied?

When converting from ETH to DOT, we need to _divide_ the multiplier by
the exchange rate, and to convert from DOT to ETH we need to _multiply_
the multiplier by the exchange rate.

> Other input parameters to the fee calculation can also deviate from
real-world values. These include substrate weights, gas prices, and so
on. Why does the multiplier introduced here not adjust those?

A single scalar multiplier won't be able to accommodate the different
volatilities efficiently. For example, gas prices are much more volatile
than exchange rates, and substrate weights hardly ever change.

So the pricing config relating to weights and gas prices should already
have some appropriate safety margin pre-applied.

# Detailed Changes:

* Added `multiplier` field to `PricingParameters`
* Outbound-queue fee is multiplied by `multiplier`
* This `multiplier` is synced to the Ethereum side
* Improved Runtime API for calculating outbound-queue fees. This API
makes it much easier to for configure parts of the system in preparation
for launch.
* Improve and clarify code documentation

Upstreamed from Snowfork#127

---------

Co-authored-by: Clara van Staden <claravanstaden64@gmail.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
claravanstaden added a commit to Snowfork/polkadot-sdk that referenced this pull request Mar 25, 2024
…nsure safety margins (paritytech#3791)

This is a cherry-pick from master of
paritytech#3727 and
paritytech#3790

Expected patches for (1.7.0):
snowbridge-pallet-ethereum-client
snowbridge-pallet-inbound-queue
snowbridge-pallet-outbound-queue
snowbridge-outbound-queue-runtime-api
snowbridge-pallet-system
snowbridge-core
claravanstaden added a commit to Snowfork/polkadot-sdk that referenced this pull request Mar 25, 2024
…ritytech#3794)

This is a cherry-pick from master of
paritytech#3790

Expected patches for (1.7.0):
snowbridge-pallet-ethereum-client
snowbridge-pallet-inbound-queue
snowbridge-pallet-outbound-queue
snowbridge-outbound-queue-runtime-api
snowbridge-pallet-system
snowbridge-core

---------

Co-authored-by: Vincent Geddes <vincent@snowfork.com>
Co-authored-by: claravanstaden <Cats 4 life!>
Co-authored-by: Vincent Geddes <vincent.geddes@hey.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
claravanstaden added a commit to Snowfork/polkadot-sdk that referenced this pull request Mar 25, 2024
…nsure safety margins (paritytech#3791)

This is a cherry-pick from master of
paritytech#3727 and
paritytech#3790

Expected patches for (1.7.0):
snowbridge-pallet-ethereum-client
snowbridge-pallet-inbound-queue
snowbridge-pallet-outbound-queue
snowbridge-outbound-queue-runtime-api
snowbridge-pallet-system
snowbridge-core
claravanstaden added a commit to Snowfork/polkadot-sdk that referenced this pull request Mar 25, 2024
…ritytech#3794)

This is a cherry-pick from master of
paritytech#3790

Expected patches for (1.7.0):
snowbridge-pallet-ethereum-client
snowbridge-pallet-inbound-queue
snowbridge-pallet-outbound-queue
snowbridge-outbound-queue-runtime-api
snowbridge-pallet-system
snowbridge-core

---------

Co-authored-by: Vincent Geddes <vincent@snowfork.com>
Co-authored-by: claravanstaden <Cats 4 life!>
Co-authored-by: Vincent Geddes <vincent.geddes@hey.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
acatangiu added a commit that referenced this pull request Mar 26, 2024
…nsure safety margins (#3814)

This is a cherry-pick from master of
#3790 and
#3727

Expected patches for (1.8.0):
snowbridge-pallet-ethereum-client
snowbridge-pallet-inbound-queue
snowbridge-pallet-outbound-queue
snowbridge-outbound-queue-runtime-api
snowbridge-pallet-system
snowbridge-core

---------

Co-authored-by: Vincent Geddes <vincent@snowfork.com>
Co-authored-by: Vincent Geddes <vincent.geddes@hey.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
acatangiu added a commit that referenced this pull request Mar 26, 2024
…nsure safety margins (#3815)

This is a cherry-pick from master of
#3790 and
#3727

Expected patches for (1.9.0):
snowbridge-pallet-ethereum-client
snowbridge-pallet-inbound-queue
snowbridge-pallet-outbound-queue
snowbridge-outbound-queue-runtime-api
snowbridge-pallet-system
snowbridge-core

---------

Co-authored-by: Vincent Geddes <vincent@snowfork.com>
Co-authored-by: Vincent Geddes <vincent.geddes@hey.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
acatangiu pushed a commit that referenced this pull request May 16, 2024
…c committee in next store period (#4482)

This is a cherry-pick from master of
[#3790 and
#3727

Expected patches for (1.7.0):
snowbridge-pallet-ethereum-client

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T15-bridges This PR/Issue is related to bridges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants