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

test(lockup): add test of unlocking schedule with termination #191

Open
wants to merge 4 commits into
base: lockup-v2.0.0-branch
Choose a base branch
from

Conversation

telezhnaya
Copy link
Contributor

Warn: it should never be merged, it's based on v2 of lockup contract

The reason of this code is to test the lockup functionality that is used most widely nowadays. We want to be sure how current lockups work

@frol
Copy link
Contributor

frol commented Dec 10, 2021

Warn: it should never be merged, it's based on v2 of lockup contract

Hmm, how is it based on v2 if the code is from the master branch?

I suggest we create lockup-v2 branch that corresponds to lockup-v2.0.0 tag, and we create a PR with lockup-v2-extra-tests branch, and once we review it, we can close the PR (while keeping the branch), and also update the README on master with the link to all those tags and branches to clarify the situation there.

Also, we need the same test on master (lockup v3), so we can at least demonstrate how the behavior is different between the versions.

assert_eq!(
contract.get_locked_amount().0,
unvested_amount_at_termination_day
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add assert on get_unlocked() == initial lockup amount - terminated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I will not add this check because both possible methods are broken
#192 (comment)
I guess they will work for this specific case, but anyway, I don't want to use them, that's a matter of coincidence that they give us the expected result here

P.S. It's still too early to review this; I will ping you separately when I finish the work here

Copy link
Contributor

Choose a reason for hiding this comment

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

As per our last discussion, let's add this assert anyway, and if it crashes (supposedly in the next test), let's comment it out and leave an explanation why we believe it is not critical

@telezhnaya telezhnaya changed the base branch from master to lockup-v2.0.0-branch December 10, 2021 14:28
@telezhnaya
Copy link
Contributor Author

test_unlocking_schedule_with_termination_and_vesting_started_before_phase2
Picture for test 1

@telezhnaya
Copy link
Contributor Author

test_unlocking_schedule_with_termination_and_vesting_started_after_phase2
Test 2

@telezhnaya
Copy link
Contributor Author

We discussed some ideas on how to improve these tests. One of the ideas was to make the same tests on the master branch.
I'm not sure it makes sense with the same setup. lockup_duration is deprecated. Moreover, not sure someone will lock the tokens before vesting starts.
I think it makes sense to write one test on master, where lockup is not connected to Phase II AND lockup goes after vesting. @frol what do you think?

This PR is ready to review.

@telezhnaya
Copy link
Contributor Author

Actually, I'm not sure whether we need these tests on master at all. I hope no one will use this contract right now; it means that we can stop spending time on it and invest it in other issues.
It was important to make the work of v2.0.0 more transparent, and I hope these tests help with it at least somehow.

@telezhnaya
Copy link
Contributor Author

Created the test for v3.0.0
9b08fe4

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