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

Fee manager #23

Merged
merged 22 commits into from
Nov 10, 2023
Merged

Fee manager #23

merged 22 commits into from
Nov 10, 2023

Conversation

alistair-singh
Copy link

Resolves: SNO-669
Snowbridge: Snowfork/snowbridge#999

@@ -605,7 +605,7 @@ parameter_types! {
}

parameter_types! {
pub TreasuryAccount: AccountId = PalletId(*b"py/trsry").into_account_truncating();
pub TreasuryAccount: AccountId = TREASURY_PALLET_ID.into_account_truncating();
Copy link
Author

Choose a reason for hiding this comment

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

Re-use the TREASURY_PALLET_ID for now. TREASURY_PALLET_ID defined to be same thing anyway:

PalletId(*b"py/trsry")

Copy link

Choose a reason for hiding this comment

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

Sounds fine. The local fees can all go to the common treasury account for BridgeHub.

Copy link
Author

Choose a reason for hiding this comment

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

let receiver = ReceiverAccount::get();
// There is an origin so split fee into parts.
if let Some(XcmContext { origin: Some(origin), .. }) = context {
if let Some(origin) = SovereignAccountOf::convert_location(origin) {
Copy link

Choose a reason for hiding this comment

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

Our other pallets on BridgeHub have all standardized on using this function for converting sibling para_id's into sovereign accounts:

https://github.com/Snowfork/snowbridge/blob/884355dad1a9e9d864805290159522f9d6b61809/parachain/primitives/core/src/lib.rs#L27

Copy link
Author

@alistair-singh alistair-singh Nov 9, 2023

Choose a reason for hiding this comment

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

Addressed in 4e2b4ff

Just a note here, I tried to do this in multiple ways but the traits contraints bled out into the implementations of control pallet, outbound queue, benchmarks and tests. So in the end I decided to just copy the function and create a test that makes sure the outputs are always the same.

I tried changing from T::AcccountId to plain AccountId but then some scale codec traits were required on our pallets and then bled into the benchmarks which are generated by macros and cannot be applied there.

primitives/core/src/lib.rs:29:29
   Compiling snowbridge-core v0.1.1 (/Users/alistairsingh/c/snowbridge/parachain/primitives/core)
error[E0277]: the trait bound `T: WrapperTypeEncode` is not satisfied
  --> primitives/core/src/lib.rs:28:31
   |
28 |     SiblingParaId::from(para_id).into_account_truncating()
   |                                  ^^^^^^^^^^^^^^^^^^^^^^^ the trait `WrapperTypeEncode` is not implemented for `T`
   |
   = note: required for `T` to implement `Encode`
   = note: required for `Sibling` to implement `AccountIdConversion<T>`
help: consider restricting type parameter `T`
   |
27 | pub fn sibling_sovereign_account<T: parity_scale_codec::WrapperTypeEncode>(para_id: ParaId) -> T {
   |                                   +++++++++++++++++++++++++++++++++++++++

error[E0277]: the trait bound `T: WrapperTypeDecode` is not satisfied
  --> primitives/core/src/lib.rs:28:31
   |
28 |     SiblingParaId::from(para_id).into_account_truncating()
   |                                  ^^^^^^^^^^^^^^^^^^^^^^^ the trait `WrapperTypeDecode` is not implemented for `T`
   |
   = note: required for `T` to implement `Decode`
   = note: required for `Sibling` to implement `AccountIdConversion<T>`
help: consider restricting type parameter `T`
   |
27 | pub fn sibling_sovereign_account<T: parity_scale_codec::WrapperTypeDecode>(para_id: ParaId) -> T {
   |                                   +++++++++++++++++++++++++++++++++++++++

For more information about this error, try `rustc --explain E0277`.
error: could not compile `snowbridge-core` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `snowbridge-core` (lib test) due to 2 previous errors


/// A `HandleFee` implementation that takes fees from `ExportMessage` XCM instructions
/// to Snowbridge and holds it in a receiver account. Burns the fees in case of a failure.
pub struct XcmExportFeeToSnowbridge<
Copy link

Choose a reason for hiding this comment

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

Since we're going to use this type in about 5 different BridgeHub runtimes, lets move it to a new crate named snowbridge-runtime-common, located in our own Snowbridge repo, in this directory: parachain/primitives/runtime-common.

Then we can the runtimes depend on our runtime-common crate.

Copy link
Author

Choose a reason for hiding this comment

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

Addressed in e3bd0e2

Comment on lines 322 to 327
XcmExportFeeToSnowbridge<
TokenLocation,
EthereumNetwork,
Self::AssetTransactor,
crate::EthereumOutboundQueue,
>,
Copy link

Choose a reason for hiding this comment

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

Nice!

@alistair-singh alistair-singh merged commit 5e361f4 into snowbridge Nov 10, 2023
3 of 7 checks passed
@alistair-singh alistair-singh deleted the alistair/fee-manager branch November 10, 2023 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants