-
Notifications
You must be signed in to change notification settings - Fork 609
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
Return lock id from BeginForceUnlock #4059
Changes from 4 commits
2813b73
c46c23d
4346ff2
2271e24
3621964
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -45,6 +45,67 @@ func (suite *KeeperTestSuite) TestBeginUnlocking() { // test for all unlockable | |||||
suite.Require().NotEqual(locks[0].IsUnlocking(), false) | ||||||
} | ||||||
|
||||||
func (suite *KeeperTestSuite) TestBeginForceUnlock() { | ||||||
p0mvn marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
// coins to lock | ||||||
coins := sdk.Coins{sdk.NewInt64Coin("stake", 10)} | ||||||
|
||||||
testCases := []struct { | ||||||
name string | ||||||
coins sdk.Coins | ||||||
unlockCoins sdk.Coins | ||||||
expectSameLockID bool | ||||||
}{ | ||||||
{ | ||||||
name: "new lock id is returned if the lock was split", | ||||||
coins: coins, | ||||||
unlockCoins: sdk.Coins{sdk.NewInt64Coin("stake", 1)}, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: while not a problem here, using the constructor is preferable because it does coin validation
Suggested change
|
||||||
expectSameLockID: false, | ||||||
}, | ||||||
{ | ||||||
name: "same lock id is returned if the lock was not split", | ||||||
coins: coins, | ||||||
unlockCoins: sdk.Coins{}, | ||||||
expectSameLockID: true, | ||||||
}, | ||||||
} | ||||||
|
||||||
for _, tc := range testCases { | ||||||
suite.SetupTest() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For each test case, let's create a separate environment for it with suite.`Run(...). Similar to: osmosis/x/gamm/keeper/pool_service_test.go Lines 302 to 303 in 6286934
I know this is not done in most of the test cases in the file. However, this is the correct way that enables a separate test environment for each table-driven test case. |
||||||
|
||||||
// initial check | ||||||
locks, err := suite.App.LockupKeeper.GetPeriodLocks(suite.Ctx) | ||||||
suite.Require().NoError(err) | ||||||
suite.Require().Len(locks, 0) | ||||||
|
||||||
// lock coins | ||||||
addr1 := sdk.AccAddress([]byte("addr1---------------")) | ||||||
suite.LockTokens(addr1, tc.coins, time.Second) | ||||||
|
||||||
// check locks | ||||||
locks, err = suite.App.LockupKeeper.GetPeriodLocks(suite.Ctx) | ||||||
suite.Require().NoError(err) | ||||||
|
||||||
for _, lock := range locks { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggest checking that the locks length is not zero here. If we don't check and it happens so that 0 locks are returned, we would skip all the validations in the for loop completely |
||||||
suite.Require().Equal(lock.EndTime, time.Time{}) | ||||||
suite.Require().Equal(lock.IsUnlocking(), false) | ||||||
|
||||||
lockID, err := suite.App.LockupKeeper.BeginForceUnlock(suite.Ctx, lock.ID, tc.unlockCoins) | ||||||
suite.Require().NoError(err) | ||||||
|
||||||
if tc.expectSameLockID { | ||||||
suite.Require().Equal(lockID, lock.ID) | ||||||
} else { | ||||||
suite.Require().Equal(lockID, lock.ID + 1) | ||||||
} | ||||||
|
||||||
// new or updated lock | ||||||
newLock, err := suite.App.LockupKeeper.GetLockByID(suite.Ctx, lockID) | ||||||
suite.Require().NoError(err) | ||||||
suite.Require().True(newLock.IsUnlocking()) | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
func (suite *KeeperTestSuite) TestGetPeriodLocks() { | ||||||
suite.SetupTest() | ||||||
|
||||||
|
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.
Lets add a comment for what is being returned. Needs to document what happens in both the case of a lock being split and not split.