Introduce bridge relayers pallet#1513
Conversation
svyatonik
added
P-Runtime
PR-audit-needed
|
So after this PR, all we need to do to solve #1318 is (I hope) to properly implement |
serban300
left a comment
serban300
left a comment
There was a problem hiding this comment.
Looks very good. I just left a couple of nits.
| let reward = maybe_reward.take().ok_or(Error::<T>::NoRewardForRelayer)?; | ||
| T::PaymentProcedure::pay_reward(&relayer, reward).map_err(|e| { | ||
| log::trace!( | ||
| target: "runtime::bridge-relayers", |
There was a problem hiding this comment.
Nit: I would define this in a const LOG_TARGET, as for the other modules.
| run_test(|| { | ||
| RelayerRewards::<TestRuntime>::insert(REGULAR_RELAYER, 100); | ||
| assert_ok!(Pallet::<TestRuntime>::claim_rewards(Origin::signed(REGULAR_RELAYER))); | ||
| assert_eq!(RelayerRewards::<TestRuntime>::get(REGULAR_RELAYER), None); |
There was a problem hiding this comment.
Can we also check if the RewardPaid event was emitted correctly ?
| RelayerRewards::<T>::mutate(relayer, |old_reward: &mut Option<T::Reward>| { | ||
| let new_reward = old_reward.unwrap_or_else(Zero::zero).saturating_add(reward); | ||
| log::trace!( | ||
| target: "T::bridge-relayers", |
There was a problem hiding this comment.
Is this desired ? Or should it be runtime::bridge-relayers ?
| messages_relayers, | ||
| received_range, | ||
| ); | ||
| if !relayers_rewards.is_empty() { |
There was a problem hiding this comment.
Nit: I wouldn't test this. If relayers_rewards is empty, almost nothing happens.
| } | ||
|
|
||
| // Update rewards to given relayers, optionally rewarding confirmation relayer. | ||
| fn schedule_relayers_rewards<T: Config>( |
There was a problem hiding this comment.
I was wondering if register_relayers_rewards or update_relayers_rewards would be a better name for this method.
There was a problem hiding this comment.
It is up to you :)
| // If delivery confirmation is submitted by this relayer, let's add confirmation fee | ||
| // from other relayers to this relayer reward. | ||
| confirmation_relayer_reward = confirmation_relayer_reward.saturating_add(reward.reward); | ||
| continue |
There was a problem hiding this comment.
Nit: I would move the schedule_relayer_reward::<T>(&relayer, relayer_reward); in the if branch above and avoid doing continue; here. Seems easier to avoid mistakes in the future.
| let mut confirmation_reward = T::Reward::try_from(reward.messages) | ||
| .unwrap_or_else(|_| Bounded::max_value()) |
There was a problem hiding this comment.
Nit: I think we can use T::Reward::saturated_from(reward.messages)
| #frame-system = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false } | ||
| #sp-core = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false } |
There was a problem hiding this comment.
Can we delete these commented lines ?
There was a problem hiding this comment.
My fault - sorry :(
svyatonik
left a comment
svyatonik
left a comment
There was a problem hiding this comment.
Thanks for the review, all your points seems legit to me!
Ok, thanks ! I'll implement them in a follow-up PR. |
|
The comments were addressed in #1547 . Marking this PR as reviewed. |
* introduce relayers pallet * add MessageDeliveryAndDispatchPaymentAdapter * plug in pallet into test runtimes * tests prototype * tests for the relayers pallet * tests for payment adapter * mint_reward_payment_procedure_actually_mints_tokens * benchmarks * remove irrelevant todo * remove redundant clone
* introduce relayers pallet * add MessageDeliveryAndDispatchPaymentAdapter * plug in pallet into test runtimes * tests prototype * tests for the relayers pallet * tests for payment adapter * mint_reward_payment_procedure_actually_mints_tokens * benchmarks * remove irrelevant todo * remove redundant clone
2 participants
closes #1503
There's still a lot of TODOs here. I'm opening it early so that anyone could ask questions and voice their opinions regarding this new pallet.
Quick intro: we have unsolved #1318 (which I'm planning to discuss once we have some kind of Rococo/Wococo bridge hubs). Till last week I was thinking that we shall not work on that completely, but actually there are parts of the code that may be worked on now. So I've branched off #1503 from #1318. I was going to add new call (
claim_rewards) and map to the messages pallet (relayer => reward) first. But then I've remembered #2486, which we also shall solve soon or later. Adding #2486 (whatever solution we'll choose) to the existing messages pallet will make it too complex, so imo it makes sense to move all relayer management code (including paying fees and rewards) to the separate pallet, which I'm introducing in this PR.