-
Notifications
You must be signed in to change notification settings - Fork 175
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
cmd, eth, server: remove claim earnings option #1637
Conversation
glog.Errorf("Need to provide amount") | ||
return | ||
} | ||
endRound, err := lpcommon.ParseBigInt(endRoundStr) |
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.
Seems like it might be ok to allow a user to still specify an endRound. Don't imagine users needing to do this, but if they have tools that depend on being able to do this for whatever reason it might be good to not break that expectation.
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 don't think there's much tooling using this, however it might be okay to force a user's hand in changing their expectation of how this functionality truly behaves currently. If those expectations are not altered somehow it will just end up incurring additional costs for those users and their expectations misalign with what optimal behaviour would be.
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 believe there are a few users that have automated claimEarnings calls via this webserver endpoint in the past. Generally, we should avoid breaking API compatibility and we should avoid making assumptions around how users use an API when we can, but I see your point about a user not realizing that there is basically no benefit in calling claimEarnings with an endRound set to anything besides the current round. Will leave this case up to you.
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 actually think it's fine because the API itself doesn't change. Users can still call the same endpoint with an argument (it will just be ignored).
If there are tools out there that automate claimEarnings they have become rather obsolete anyway even it's for up until the current round.
If a user needs funds it's more economical to perform a staking action for example unbond, or withdraw fees directly.
From that perspective it make even make sense to just remove the endpoint entirely, while it might misalign with user expectations (we can clarify this in the release notes) it protects users from operating inefficiently.
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 after squashing fixup commits
c6cb779
to
04d3871
Compare
What does this pull request do? Explain your changes. (required)
Removes the option from the CLI to manually claim earnings
Specific updates (required)
BondingManager
contract bindings directlyHow did you test each of these updates (required)
Does this pull request close any open issues?
Fixes #1635
Checklist:
./test.sh
pass