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

Relayer rewards paid to specified location account #6578

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

claravanstaden
Copy link
Contributor

Adds the required functionality for relayers to accumulate and claim rewards.

@bkontur and @acatangiu, I just lifted and shifted the code that was in the snowbridge rewards pallet into the bridge-relayers pallet. I looked at the existing code (to see if I can reuse it) and I think the main shortcoming of the existing claim_rewards method is the ability to claim the rewards to destination account location on AH (instead of using the extrinsic origin as destination account). And we won't use rewards_account_params, since we mint Weth on AH. Let me know your thoughts, and which option your prefer:

  1. Use this code as I am submitting it now (with some cleanup - I'll wrap all the newly added pallet configs in a trait)
  2. Add a parameter to claim_rewards to specify a target account location

@claravanstaden claravanstaden changed the title Relayer rewards paid into specified location account Relayer rewards paid to specified location account Nov 21, 2024
@bkontur
Copy link
Contributor

bkontur commented Nov 22, 2024

@claravanstaden Nice! I’m glad you’re starting with this. I’m marking it as part of [this issue](https://github.com/orgs/paritytech/projects/145/views/8?pane=issue&itemId=84899273).

I have some ideas on how to make this pallet more generic to cover all our use cases (collecting and claiming rewards regardless of where the messaging queues/pallets are deployed—whether on AH or BH). I believe a single generic RelayerRewards map with a "reward_discriminator" and one extrinsic for claiming (plus a migration for P/K rewards) should suffice. Regarding P/K rewards or Snowbridge rewards, it should just be a matter of configuring the pallet where it’s deployed.

I’d like to prepare a more detailed final design for this. How time-sensitive is this for you? What is your deadline?

@claravanstaden
Copy link
Contributor Author

Thanks @bkontur, that sounds great! Would reward_discriminator describe the reward asset time.

We need this functionality in stable-2412. Hope that is feasible for you. I can aid in the implementation too. :)

@acatangiu
Copy link
Contributor

We need this functionality in stable-2412. Hope that is feasible for you. I can aid in the implementation too. :)

stable-2412 has already been branched off since early Nov (see cutoff date here), it is now in QA and audit period. The changes/functionality you are adding with this PR should also be tested and audited so it should go in before QA and audit period. Meaning you missed stable-2412.

If push comes to shove and it is critical to make this release you could force it in, but then I would not change existing code to avoid introducing regressions that we don't have time to test or catch.

@vgeddes
Copy link
Contributor

vgeddes commented Nov 22, 2024

@claravanstaden actually we are aiming for the stable2503 release, soft code-freeze on Dec 26.

@claravanstaden
Copy link
Contributor Author

@acatangiu @bkontur so we are targeting stable2503, sorry for the mistake on my part. Would it be feasible to do a generic version of the rewards extrinsic? I can do some of the work myself if it helps.

@claravanstaden claravanstaden marked this pull request as ready for review December 18, 2024 09:18
@paritytech-review-bot paritytech-review-bot bot requested a review from a team December 18, 2024 09:19
@bkontur
Copy link
Contributor

bkontur commented Jan 13, 2025

@claravanstaden I will definitely look into it deeply this week because I will also need it :)

@bkontur bkontur self-requested a review January 13, 2025 10:26
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.

4 participants