chore: Refactor and Add tests for AddToExistingLock
#1979
Conversation
There was a problem hiding this comment.
LGTM - thanks for fighting all the weird edge cases!
I think that it makes sense to return an error when there are no locks in AddToExistingLock, despite this change being state-breaking. However, leaving the final call to you @mattverse 👍
|
agreed with you on adding error when there's no lock druing |
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
There was a problem hiding this comment.
LGTM. Thanks for working out all edge cases here @mattverse
| // curBalance := suite.App.BankKeeper.GetAllBalances(cacheCtx, addr1) | ||
| _, err = suite.App.LockupKeeper.AddTokensToLockByID(cacheCtx, 1111, addr1, addCoins) | ||
| suite.Require().Error(err) | ||
| if tc.expectAddTokensToLockSuccess { |
There was a problem hiding this comment.
What is the difference between expectError and expectAddTokensToLockSuccess? Can we use only expectError?
There was a problem hiding this comment.
good catch, the original intent was to create a separate case for lock failing and adding to lock failing, but these can be grouped to one. Changed to removing the field!
Co-authored-by: Roman <roman@osmosis.team>
…-labs/osmosis into mattverse/lock-testing-2
sub-component of: #1838
What is the purpose of the change
AddToExistingLockhad a bad design of returning array of locks, and then only using the first index out of the given array.There's no need to return an array here, but simply return the lock that we've added the token to. This PR changes the returning values of
AddToExistingLockfrom an array to a single lock(not pointer referenced).This PR also includes changing tests for
AddToExistingLockto be TDD, adding some edge cases to the tests.Brief Changelog
AddToExistingLockAddToExistingLockto be TDDTesting and Verifying
This change added tests
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? no