Refactor `lock` method #1936

mattverse · 2022-07-01T05:32:19Z

Sub component of: #1838

What is the purpose of the change

The lock method in the lockup module was a public method before current change, which was an imo dangerous design, as it is and should be only called by either of the two methods

CreateLock
AddTokensToLockByID

That being said, there shouldn't be any cases where the Lock method is called directly, but called only through the two entrypoints mentioned above, not to mention the code duplication that we had for the two methods above.

This PR changes the Lock method into a private method and refactors code duplication logic as well.

Brief Changelog

API change for the Lock method in x/lockup module.

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? no
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
How is the feature or change documented? not documented

x/lockup/keeper/lock.go

ValarDragon · 2022-07-01T14:29:36Z

x/lockup/keeper/lock.go

-	err = k.addTokenToLock(ctx, lock, coin)
-	if err != nil {
-		return nil, err


This looks wrong? addTokenToLock did more, e.g. handling of the main accumulation store? Why was this deleted?

Oh the claim is that all functionality is there between the synth lock line, and lock.

Why not keep addTokenToLock and just have that function as a subcomponent call lock? (But retain the synth lock functionality)

That would be maintaining the standard in this package of having the exported method be doing validation logic & hook / return type logic, and the private method doing the state change

I still don't fully follow the logic here. There are two calls that addTokenToLock makes that are not related to the synth lock related accumulation store update, which are:

setLock to update state with the new lock instance:

osmosis/x/lockup/keeper/lock.go

Line 100 in 8f018a6

err := k.setLock(ctx, *lock)

Update to accumulationStore:

osmosis/x/lockup/keeper/lock.go

Line 106 in 8f018a6

k.accumulationStore(ctx, coin.Denom).Increase(accumulationKey(lock.Duration), coin.Amount)

How/where are these being accounted for in the new implementation?

Oops, I see the lock call above now. The change that adds an addition parameter to lock also makes sense now. Clever refactor :)

@ValarDragon wdym by ? Are you referring to synth lock accumulation store part?

(But retain the synth lock functionality)

x/lockup/keeper/lock.go

p0mvn · 2022-07-01T15:50:06Z

x/lockup/keeper/lock_test.go

@@ -144,47 +144,17 @@ func (suite *KeeperTestSuite) TestUnlockPeriodLockByID() {
 	suite.Require().Len(locks, 0)
 }

-func (suite *KeeperTestSuite) TestLock() {


Does it make sense to do either or both:

have additional tests for CreateLock that would cover the removed logic

use the export_test.go trick that will allow us to

export lock for tests only and

keep testing the now unexported lock method

Ah missed the tests! thanks for catching this! Just added this in the most recent commit!

AlpinYukseloglu

Left a few clarifying questions but overall LGTM! Love this refactor, cleans things up quite a bit :)

x/lockup/keeper/lock.go

AlpinYukseloglu · 2022-07-03T20:28:35Z

x/lockup/keeper/lock.go

-	err = k.addTokenToLock(ctx, lock, coin)
-	if err != nil {
-		return nil, err


I still don't fully follow the logic here. There are two calls that addTokenToLock makes that are not related to the synth lock related accumulation store update, which are:

setLock to update state with the new lock instance:

osmosis/x/lockup/keeper/lock.go

Line 100 in 8f018a6

err := k.setLock(ctx, *lock)

Update to accumulationStore:

osmosis/x/lockup/keeper/lock.go

Line 106 in 8f018a6

k.accumulationStore(ctx, coin.Denom).Increase(accumulationKey(lock.Duration), coin.Amount)

How/where are these being accounted for in the new implementation?

x/lockup/keeper/lock.go

AlpinYukseloglu · 2022-07-03T20:33:25Z

x/lockup/keeper/lock.go

-	err = k.addTokenToLock(ctx, lock, coin)
-	if err != nil {
-		return nil, err


Oops, I see the lock call above now. The change that adds an addition parameter to lock also makes sense now. Clever refactor :)

x/lockup/keeper/lock.go

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

…labs/osmosis into mattverse/refactor-lock

mattverse · 2022-07-05T07:29:05Z

merging this upon being blocker for future work for adding testing for lockup!

ValarDragon · 2022-07-05T13:14:18Z

I'm a bit concerned about this change / haven't convinced myself they are at functionality parity

mattverse · 2022-07-05T14:22:22Z

Apologies if I went ahead and rushed into merging this, I supposed there weren't extra reviews.

Is there something you're concerned about or would like to give a review on? We can either revert this commit or create a new PR depending on the review

mattverse · 2022-07-08T05:24:11Z

A Note to reviewers to clarify no logic has been removed / changed per this PR

Previous logic for AddTokensToLockByID

SendCoinsFromAccountToModule
addTokenToLock
- lock.Coins = lock.Coins.Add(coin)
- setLock
- accumulation store increase
- synth denom accumulation store increase
- hooks.AfterAddTokensToLock
hooks.OnTokenLocked

Refactored Logic for AddTokensToLockByID

lock.Coins = lock.Coins.Add(coin)
lock
- SendCoinsFromAccountToModule
- setLock
- accumulation store increase
- hooks.OnTokenLocked
synth denom accumulation store increase
hooks.AfterAddTokensToLock

Previous logic for CreateLock

Lock
- SendCoinsFromAccountToModule
- store.Set(lock)
- addlockRefs
- accumulation store increase
- hooks.OnTokenLocked
setLastLockID

Refactored Logic for CreateLock

lock
- SendCoinsFromAccountToModule
- setLock
- accumulation store increase
- hooks.OnTokenLocked
addLockRefs
SetLastLockID

* Add lock method refactor * Delete duplciated testing * Update x/lockup/keeper/lock.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * Add tests implement feedback from code review * Add test cases Co-authored-by: Aleksandr Bezobchuk <[email protected]> (cherry picked from commit c115593) # Conflicts: # x/lockup/keeper/admin_keeper_test.go # x/lockup/keeper/lock.go

* Refactor `lock` method (#1936) * Add lock method refactor * Delete duplciated testing * Update x/lockup/keeper/lock.go Co-authored-by: Aleksandr Bezobchuk <[email protected]> * Add tests implement feedback from code review * Add test cases Co-authored-by: Aleksandr Bezobchuk <[email protected]> (cherry picked from commit c115593) # Conflicts: # x/lockup/keeper/admin_keeper_test.go # x/lockup/keeper/lock.go * Fix merge conflict * Merge Co-authored-by: Matt, Park <[email protected]> Co-authored-by: mattverse <[email protected]>

This reverts commit d733f64.

* Revert "x/lock: Fix `ExtendLockup` API (backport #1937) (#2030)" This reverts commit bd32316. * Revert "Refactor `lock` method (backport #1936) (#2029)" This reverts commit d733f64. * Update changelog * Update changelog

mattverse added 2 commits July 1, 2022 14:16

Add lock method refactor

639178c

Delete duplciated testing

c9bfaeb

mattverse requested a review from a team July 1, 2022 05:32

github-actions bot added the C:x/lockup label Jul 1, 2022

mattverse mentioned this pull request Jul 1, 2022

x/lockup - internal audit #1838

Closed

7 tasks

alexanderbez approved these changes Jul 1, 2022

View reviewed changes

x/lockup/keeper/lock.go Show resolved Hide resolved

ValarDragon reviewed Jul 1, 2022

View reviewed changes

x/lockup/keeper/lock.go Show resolved Hide resolved

ValarDragon reviewed Jul 1, 2022

View reviewed changes

x/lockup/keeper/lock.go Show resolved Hide resolved

p0mvn reviewed Jul 1, 2022

View reviewed changes

p0mvn mentioned this pull request Jul 1, 2022

lint: investigate and fix errcheck linter #1940

Closed

AlpinYukseloglu reviewed Jul 3, 2022

View reviewed changes

mattverse and others added 4 commits July 4, 2022 11:55

Update x/lockup/keeper/lock.go

d0cfc31

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

Add tests implement feedback from code review

6413243

Merge branch 'mattverse/refactor-lock' of https://github.com/osmosis-…

15b4bcb

…labs/osmosis into mattverse/refactor-lock

Add test cases

73643bd

AlpinYukseloglu approved these changes Jul 5, 2022

View reviewed changes

mattverse merged commit c115593 into main Jul 5, 2022

mattverse deleted the mattverse/refactor-lock branch July 5, 2022 07:29

mattverse added the A:backport/v10.x backport patches to v10.x branch label Jul 12, 2022

mergify bot mentioned this pull request Jul 12, 2022

Refactor lock method (backport #1936) #2029

Merged

mattverse restored the mattverse/refactor-lock branch July 12, 2022 06:44

mattverse added A:backport/v10.x backport patches to v10.x branch api-breaking Breaking changes that impact API only (not state machine). and removed A:backport/v10.x backport patches to v10.x branch labels Jul 12, 2022

ValarDragon added a commit that referenced this pull request Aug 1, 2022

Revert "Refactor lock method (backport #1936) (#2029)"

242a0f0

This reverts commit d733f64.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `lock` method #1936

Refactor `lock` method #1936

mattverse commented Jul 1, 2022 •

edited

Loading

ValarDragon Jul 1, 2022

ValarDragon Jul 1, 2022

AlpinYukseloglu Jul 3, 2022

AlpinYukseloglu Jul 3, 2022

mattverse Jul 4, 2022 •

edited

Loading

p0mvn Jul 1, 2022 •

edited

Loading

mattverse Jul 4, 2022 •

edited

Loading

AlpinYukseloglu left a comment

AlpinYukseloglu Jul 3, 2022

AlpinYukseloglu Jul 3, 2022

mattverse commented Jul 5, 2022

ValarDragon commented Jul 5, 2022

mattverse commented Jul 5, 2022

mattverse commented Jul 8, 2022

Refactor lock method #1936

Refactor lock method #1936

Conversation

mattverse commented Jul 1, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Documentation and Release Note

ValarDragon Jul 1, 2022

Choose a reason for hiding this comment

ValarDragon Jul 1, 2022

Choose a reason for hiding this comment

AlpinYukseloglu Jul 3, 2022

Choose a reason for hiding this comment

AlpinYukseloglu Jul 3, 2022

Choose a reason for hiding this comment

mattverse Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

p0mvn Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

mattverse Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

AlpinYukseloglu left a comment

Choose a reason for hiding this comment

AlpinYukseloglu Jul 3, 2022

Choose a reason for hiding this comment

AlpinYukseloglu Jul 3, 2022

Choose a reason for hiding this comment

mattverse commented Jul 5, 2022

ValarDragon commented Jul 5, 2022

mattverse commented Jul 5, 2022

mattverse commented Jul 8, 2022

Refactor `lock` method #1936

Refactor `lock` method #1936

mattverse commented Jul 1, 2022 •

edited

Loading

mattverse Jul 4, 2022 •

edited

Loading

p0mvn Jul 1, 2022 •

edited

Loading

mattverse Jul 4, 2022 •

edited

Loading