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.

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

Support partial withdraw for unlocked tokens #20

Merged
merged 1 commit into from
Jul 9, 2024
Merged

Conversation

codchen
Copy link
Collaborator

@codchen codchen commented Jul 3, 2024

Describe your changes and provide context

Allow passing in an amount field that specifies the upper limit of unlocked tokens one wishes to initiate withdrawal for. Specifically,

  • If the total amount of unlocked tokens is smaller than amount, the total amount would be withdrawn
  • If the total amount of unlocked tokens is larger than amount, amount would be withdrawn, and the first slot of the remaining tranche will have its amount deducted accordingly.

Testing performed to validate your change

unit test
local sei

@codchen codchen requested a review from philipsu522 July 3, 2024 08:56
Copy link
Contributor

@philipsu522 philipsu522 left a comment

Choose a reason for hiding this comment

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

tests look great! left some comments

Comment on lines +31 to +35
VESTING_AMOUNTS.save(storage, &vec![])?;
VESTING_TIMESTAMPS.save(storage, &vec![])?;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want these to be mutable? or do we want to introduce more state to track? i'm fine either way (we can always query the contract state in the past)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah we always have the instantiate state so I feel like we can just reuse these for modifications

src/vesting.rs Outdated
@@ -91,11 +120,26 @@ mod tests {
.unwrap();
VESTING_AMOUNTS.save(deps_mut.storage, &vec![10]).unwrap();

assert_eq!(10, collect_vested(deps_mut.storage, now).unwrap());
assert_eq!(10, collect_vested(deps_mut.storage, now, 15).unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

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

so the current logic allows one to specify withdrawing more than is vested? would it be better to force the user to specify the exact amount and error out instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll defer to you on that since you'd often be the one initiating the withdrawal (like if getting exact amount is not hard then yeah we can add a check here)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

@codchen codchen force-pushed the partial-withdrawal branch from bdad659 to 34c6d0f Compare July 9, 2024 03:26
@codchen codchen merged commit f39cabb into main Jul 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants