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

Snowbridge V2 - Unified Rewards #1

Open
wants to merge 40 commits into
base: outbound-queue-v2
Choose a base branch
from

Conversation

claravanstaden
Copy link
Owner

@claravanstaden claravanstaden commented Oct 23, 2024

Based on yrong#3 and yrong#4.

@yrong
Copy link

yrong commented Oct 24, 2024

Would suggest to not include the changes of inbound/outbound queue V2 and make the diff a bit easier.

@claravanstaden
Copy link
Owner Author

Would suggest to not include the changes of inbound/outbound queue V2 and make the diff a bit easier.

I need to base my changes on top of your work though, right?

@claravanstaden
Copy link
Owner Author

claravanstaden commented Oct 24, 2024

@yrong I can merge my changes into your branch. What makes it tricky is your changes are on 2 separate branches (inbound and outbound). Can we perhaps merge your changes into your v2 branch? I tried merging your 2 branches locally but had some compiler errors after the merge.

@claravanstaden claravanstaden changed the base branch from master to outbound-queue-v2 October 24, 2024 12:01
@claravanstaden
Copy link
Owner Author

claravanstaden commented Oct 24, 2024

@yrong @vgeddes this PR is ready for a first review. The diff includes Ron's yrong#3 changes. Once both the inbound and outbound work is in one branch I'll base my branch off it.

TODO:

  • Update unit tests

@@ -165,3 +159,102 @@ fn send_weth_from_asset_hub_to_ethereum_by_executing_raw_xcm() {
);
});
}

#[test]
fn claim_rewards_works() {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Added E2E tests over here.

@yrong
Copy link

yrong commented Oct 24, 2024

Once both the inbound and outbound work is in one branch I'll base my branch off it.

@claravanstaden I've rebased the previous change for inbound-queue-v2 in yrong#3

So maybe we can continue the work for inbound queue based on that.

# Conflicts:
#	Cargo.lock
#	bridges/snowbridge/pallets/inbound-queue-v2/src/lib.rs
#	bridges/snowbridge/pallets/outbound-queue-v2/src/lib.rs
#	bridges/snowbridge/primitives/core/Cargo.toml
#	bridges/snowbridge/primitives/core/src/lib.rs
#	bridges/snowbridge/primitives/router/src/inbound/mod.rs
#	bridges/snowbridge/primitives/router/src/inbound/v1.rs
#	cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/snowbridge_v2.rs
#	cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/bridge_to_ethereum_config.rs
# Conflicts:
#	bridges/snowbridge/pallets/inbound-queue-v2/src/lib.rs
#	bridges/snowbridge/pallets/inbound-queue-v2/src/mock.rs
#	cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/bridge_to_ethereum_config.rs
bridges/snowbridge/pallets/rewards/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/rewards/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/rewards/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/rewards/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/rewards/src/lib.rs Outdated Show resolved Hide resolved
});

let dest = Location::new(1, [Parachain(T::AssetHubParaId::get().into())]);
let (_xcm_hash, _) = send_xcm::<T::XcmSender>(dest, xcm).map_err(Error::<T>::from)?;
Copy link

Choose a reason for hiding this comment

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

Suggested change
let (_xcm_hash, _) = send_xcm::<T::XcmSender>(dest, xcm).map_err(Error::<T>::from)?;
let (_xcm_hash, xcm_delivery_fee) = send_xcm::<T::XcmSender>(dest, xcm).map_err(Error::<T>::from)?;

The cost of delivering the message to AH needs to taken from the account of the origin. If they don't have the funds to cover the fee, then the process should be aborted.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@vgeddes if I take the returned delivery fee from send_xcm, the AH XCM instruction has already been sent and aborting the process would not make sense, right? So should I rather just check the relayer balance on DOT is above a sensible (configured) number?

Copy link

Choose a reason for hiding this comment

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

Can get delivery cost with

let (_, fee) = T::XcmSender::validate(&mut Some(dest), &mut Some(xcm))

Snowfork#142 FYI.

bridges/snowbridge/pallets/rewards/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/rewards/src/lib.rs Outdated Show resolved Hide resolved
@claravanstaden claravanstaden marked this pull request as ready for review October 31, 2024 13:02
@claravanstaden
Copy link
Owner Author

claravanstaden commented Oct 31, 2024

@yrong @vgeddes I have made the following changes:

  • Moved the rewards pallet code to the bridge relayers module
  • Made hard-coded values configurable
  • PR comments in this review

@yrong
Copy link

yrong commented Nov 3, 2024

Moved the rewards pallet code to the bridge relayers module

Just curious what's the benefit for doing this? Is there some common codes that the two modules can share?

@claravanstaden
Copy link
Owner Author

Just curious what's the benefit for doing this? Is there some common codes that the two modules can share?

@yrong the bridges team requested we reuse their pallet instead of creating another relayer rewards pallet, even if we don't reuse anything right now.

@claravanstaden claravanstaden deleted the branch outbound-queue-v2 November 5, 2024 11:45
@claravanstaden claravanstaden reopened this Nov 5, 2024
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