-
Notifications
You must be signed in to change notification settings - Fork 150
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
Implement accounts/{accountId}/staking-payouts
#217
Conversation
Remove unused comment Update comments and org Update response structure Remove validator count
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only reviewed the Rust part. Looks generally good to me. However, I thought maybe it would be better to put all Rust stuff in one package and give it a more general name rather than having one per function. Otherwise this creates a lot of code duplication and friction when adding new stuff.
You could just put those functionalities into different submodules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few nitpicks...looks great!
@@ -0,0 +1,203 @@ | |||
use crate::debug; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should have used git mv somewhere to avoid this. These are not new, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
calc_payout | ||
} | ||
|
||
pub fn calc_payout( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could look into making a PR to substrate and so that pallet-staking exposes a function that only does the calculation, and then you simply call into that here, rather than duplicating code.
Given that it can be done with sensible changes to staking, I will totally support this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% support this as it will make future runtime upgrades much easier to support.
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Closes #216
In this PR we add
accounts/{accountId}/staking-payouts
, and Rust code to do payout calculations. The@polkadot/calc-fee
wasm-pack built package becomes@substrate/calc
as it now incorporates both fee calculation and payout calculation.The main things to consider for this PR:
TODO:
openapi-proposal.yaml
to matchFollow up PRs