Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
lockup: Allow rebonding of unbonding tokens #4713
lockup: Allow rebonding of unbonding tokens #4713
Changes from 54 commits
79bbd63
8a84879
b438068
6716f76
780b8ae
a72d187
bf195bc
016bb8f
52055a6
3bfef36
a0d48b0
c3d07e0
f986221
d0db1c5
afc0f71
87e34c1
202a0ac
31bd48f
f1d17d7
a8ebba5
f5fdd19
2e472ae
50ac563
81b01f3
e5c3283
b065be9
e0e7d35
6a3df68
0aae870
c9d5a78
8e7bbce
d272db2
adb12ce
a2f5dd3
fdd8013
0759709
89e57c6
470075b
541c433
d2b955e
f051a3e
bffa4d1
e82137c
dce2cff
71f740a
eef1841
0816b50
ef0be05
7599444
be4b5aa
8e3ab60
ebf4701
5c8c447
4b1887f
141ab8b
bd7ce15
1a93e64
4f185de
8a71c81
458167f
fa5559b
6b193ba
85efccc
bcafe70
2736f8e
fb705a3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Would you be able to add an additional set of test cases (or extend these ones by adding a new test field for time elapsed) that covers cases where time has elapsed since the lock started unlocking? Specifically, it is important we test behavior around when:
It would also be great to have cases covering behavior where
BeginUnlock
is run on part (not all) oflockedCoins
, as this triggers a different logic branch that usesSplitLock
that is currently untested.Finally, we should also have test cases that include the creation of other locks and assert that they are unchanged.
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.
hey @AlpinYukseloglu! Those are great points, thank you for your review!
Note: I changed the logic of tests to make it as readable as possible. Now, in a test case struct I include so called
setupFunctions
that are being ran prior to testing. These include: creation of initial lock, unlocking the lock, creating synthetic lockup, etc.I addressed your comments in latest commits, here is a breakdown of everything for an easier review process:
Though, I have one concern about exactly unlocked lock (2). (2) works because I run
EndBlocker
in the test prior to rebonding a lock. Because of that, it automatically removes the lock if the current block time is exactly theendTime
of a lock (matured).However, in real scenario, when transactions of a block with block time equal to
endTime
of an unlocking lock are being executed, if someone tries to rebond this lock, they will be able to do so, since it was not yet removed from the store. I am a little confused as to why in case (2), we should fail? Since unlocking of maturing locks happens inEndBlocker
, I do not see the reason for forbidding case (2) from passingThere 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.
tagging for re-review of the new test
cc: @mattverse @p0mvn
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.
Adding on case (2): basically, the test works right now by simulating an unreal scenario - by calling endBlocker and then running rebonding logic both with the same blocktime is essentially equivalent to a scenario, when two consecutive blocks have the same block time
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 think case 2 correct way would be being able to rebond if endblocker has not been called yet