-
Notifications
You must be signed in to change notification settings - Fork 18
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
TokenStaking: method to increase authorization to max #161
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: David Núñez <[email protected]> Co-authored-by: Michalina <[email protected]>
…m governance contracts Co-authored-by: Michalina <[email protected]>
…not paused, not disabled)
0dacff1
to
0bef9ad
Compare
Solidity API documentation preview available in the artifacts of the https://github.com/threshold-network/solidity-contracts/actions/runs/7037152523 check. |
Solidity API documentation preview available in the artifacts of the https://github.com/threshold-network/solidity-contracts/actions/runs/7037171903 check. |
Solidity API documentation preview available in the artifacts of the https://github.com/threshold-network/solidity-contracts/actions/runs/7053764375 check. |
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.
LGTM! 👌
The base branch was changed.
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.
LGTM, just a minor comment
if (amount == tStake) { | ||
stakingProviderStruct.authorizedApplications.push(application); | ||
authorizedApplications++; | ||
} |
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.
Does it make sense to put this if block inside the if (amount > 0)
block below? This can remove the need to make this check when amount == 0, which in any case wouldn't imply pushing a new app.
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'm not fan of nested if(s) and for(s) so basically here I'm trying to make it easier to read in expense of one additional check. If you think it's not great approach then I can change it
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.
It's not a strong opinion. If you prefer it this way, I'm fine with that 👍
Based on #159
This new method will be useful for new stakers especially when
TokenStaking
will have more than 2 approved applications