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

feat: add StakerRewardClaim proxy #934

Merged
merged 7 commits into from May 17, 2023
Merged

Conversation

gitofdeepanshu
Copy link
Contributor

Pull Request Summary

Added a new proxy type StakerRewardClaim.
Only allows claim_staker call (and pallet-utility calls) to be executed.
This is less permissive than existing DappsStaking proxy type and is more appropriate for remote reward claim.

related to #856

Check list

  • updated Astar official documentation
  • updated spec version
  • updated semver

@gitofdeepanshu gitofdeepanshu added runtime This PR/Issue is related to the topic “runtime”. shiden related to shiden runtime shibuya related to shibuya labels May 12, 2023
@@ -91,7 +91,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
spec_name: create_runtime_str!("local"),
impl_name: create_runtime_str!("local"),
authoring_version: 1,
spec_version: 1,
spec_version: 2,
Copy link
Member

Choose a reason for hiding this comment

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

We always keep this one on 1, given it's just local chain.

Copy link
Member

@shaunxw shaunxw left a comment

Choose a reason for hiding this comment

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

Probably not in the scope of this PR as there is no setup yet, but it would be good to have runtime integration tests, to verify configs like this work as expected.

@gitofdeepanshu
Copy link
Contributor Author

Probably not in the scope of this PR as there is no setup yet, but it would be good to have runtime integration tests, to verify configs like this work as expected.

I agree, we have some tests for ProxyType on shiden and should include tests with every runtime change.

Signed-off-by: Deepanshu Hooda <hoodaisdeepanshu@gmail.com>
Signed-off-by: Deepanshu Hooda <hoodaisdeepanshu@gmail.com>
Signed-off-by: Deepanshu Hooda <hoodaisdeepanshu@gmail.com>
Signed-off-by: Deepanshu Hooda <hoodaisdeepanshu@gmail.com>
Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

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

Good to go, I'd suggest to apply the cosmetic comments since it's very little effort.

@@ -1929,3 +1937,256 @@ cumulus_pallet_parachain_system::register_validate_block! {
BlockExecutor = cumulus_pallet_aura_ext::BlockExecutor::<Runtime, Executive>,
CheckInherents = CheckInherents,
}
#[cfg(test)]
Copy link
Member

Choose a reason for hiding this comment

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

Nit-picky: a new line would be nice.

For the follow-up, I'd suggest creating an item to see how to build up on these runtime tests.
I'm not sure we should mock everything, this seems more appropriate for semi-live testing via RPC, as @shaunxw already suggested.

Comment on lines +909 to +910
}
ProxyType::StakerRewardClaim => {
Copy link
Member

Choose a reason for hiding this comment

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

Let's be consistent with the comments.
Either all should have them or none.

Also for the enum values - e.g. in Shiden we have enum comments, but in Shibuya and local we don't.

@Dinonard Dinonard merged commit 938a8af into master May 17, 2023
6 checks passed
@Dinonard Dinonard deleted the feat/stakerRewardClaim-proxy branch May 17, 2023 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runtime This PR/Issue is related to the topic “runtime”. shibuya related to shibuya shiden related to shiden runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants