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.

Sign up for GitHub

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

Jump to bottom

fix: Make LQTY staking claiming consistent across endpoints #81

Merged
merged 3 commits into from
Nov 25, 2024
Merged

fix: Make LQTY staking claiming consistent across endpoints #81

merged 3 commits into from
Nov 25, 2024

Conversation

bingen
Copy link
Contributor

@bingen bingen commented Nov 20, 2024 •
edited
Loading

TODO:

  • Add tests for all new possible combinations

@bingen bingen self-assigned this Nov 20, 2024
danielattilasimon
danielattilasimon approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@danielattilasimon danielattilasimon left a comment

Choose a reason for hiding this comment

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

Looks good!

I find it a bit weird that the defaults for _doSendRewards are different in depositLQTY() and withdrawLQTY(). Or was that to appease the tests?

@bingen
Copy link
Contributor Author

bingen commented Nov 21, 2024

Or was that to appease the tests?

Yes, mostly. Besides, I didn’t find it so weird. They have opposite directions. In general it seems better to not claim by default, to avoid taxable events for instance, but on full withdrawal it seems weird to leave only rewards.
Anyway, having the same for both sounds good to me too, probably better, but we can fix it in a separate PR as it seems much less urgent.

@danielattilasimon danielattilasimon deleted the branch main November 22, 2024 03:55
@danielattilasimon
Copy link
Contributor

Sorry, Github automatically closed the PR when I deleted the D-A1 branch, which had been merged. I didn't realize this PR was still based on that branch.

@danielattilasimon danielattilasimon changed the base branch from D-A1 to main November 22, 2024 04:06
@bingen bingen merged commit e5f4e5f into main Nov 25, 2024
6 checks passed
@danielattilasimon danielattilasimon deleted the cs-014 branch January 19, 2025 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielattilasimon danielattilasimon danielattilasimon approved these changes

@RickGriff RickGriff Awaiting requested review from RickGriff

Assignees

@bingen bingen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bingen @danielattilasimon