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

Allow partial bonds claim #642

Merged
merged 7 commits into from
Mar 3, 2023
Merged

Allow partial bonds claim #642

merged 7 commits into from
Mar 3, 2023

Conversation

grandizzy
Copy link
Contributor

@grandizzy grandizzy commented Mar 3, 2023 •
edited
Loading

  • allow kicker to withdraw parts of claimable bonds amount
  • add amount param to withdrawBonds function (The max amount to withdraw from auction bonds. Constrained by claimable amounts and liquidity)
  • the amount to transfer is the min of passed amount parameter, claimable balance and quote token balance of the pool
  • if amount to transfer is 0 function reverts with InsufficientLiquidity
  • added new BondWithdrawn event containing owner address, recipient to receive amount address and amount
  • added tests (didn't manage to replicate a scenario where liquidity / reserves of the pool less than what kicker is entitled to claim)

@grandizzy grandizzy changed the title Claim part of bonds Allow partial bonds claim Mar 3, 2023
@grandizzy grandizzy marked this pull request as ready for review March 3, 2023 09:56
mattcushman
mattcushman approved these changes Mar 3, 2023
View reviewed changes
Copy link
Contributor

@mattcushman mattcushman left a comment

Choose a reason for hiding this comment

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

lgtm

prateek105
prateek105 reviewed Mar 3, 2023
View reviewed changes
src/base/Pool.sol Outdated
function withdrawBonds(address recipient_) external {
function withdrawBonds(
address recipient_,
uint256 amount_
Copy link
Contributor

Choose a reason for hiding this comment

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

we can rename amount_ to maxAmount_ to be consistent with rest of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed with 7e5e546

prateek105
prateek105 approved these changes Mar 3, 2023
View reviewed changes
Copy link
Contributor

@prateek105 prateek105 left a comment

Choose a reason for hiding this comment

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

LGTM! Just added one comment

@grandizzy grandizzy merged commit f6f857b into develop Mar 3, 2023
@grandizzy grandizzy deleted the claim-part-of-bonds branch March 3, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattcushman mattcushman mattcushman approved these changes

@prateek105 prateek105 prateek105 approved these changes

@EdNoepel EdNoepel Awaiting requested review from EdNoepel

@ith-harvey ith-harvey Awaiting requested review from ith-harvey

@MikeHathaway MikeHathaway Awaiting requested review from MikeHathaway

Assignees
No one assigned
Labels
Priority: High Scope: Bug fix Status: Ready for Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@grandizzy @prateek105 @mattcushman