Add lockup duration edit method #1253

mconcat · 2022-04-14T14:26:03Z

Description

Closes: #164

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

remove gw file fdsa

ValarDragon · 2022-04-14T14:31:29Z

Ah sweet! Lets add a hook for this?

Also thoughts on renaming EditLockup to ExtendLockup?

mconcat · 2022-04-14T14:45:57Z

Ah sweet! Lets add a hook for this?

Also thoughts on renaming EditLockup to ExtendLockup?

I was thinking about using this method for editing other part of the lockups in the future(e.g. cancelling unlock) so we can reduce the total number of messages. If you think they should be in different message, I agree ExtendLockup is a better name!

ValarDragon · 2022-04-15T00:02:50Z

I think they should be different messages!

(It is an interesting API question if CancelUnlock should be expected behavior of extend lockup on an unbonding lockup, but I think thats a future question)

proto/osmosis/lockup/tx.proto

x/lockup/keeper/lock.go

x/lockup/keeper/msg_server.go

x/lockup/types/msgs.go

mattverse

Can we also add tests for EditLockup to check if the method has changed the lock as expected apart from the accumulation store check?

x/lockup/keeper/lock.go

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

codecov-commenter · 2022-04-18T14:52:56Z

Codecov Report

Merging #1253 (dba0383) into main (f56fbe5) will decrease coverage by 1.37%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##             main    #1253      +/-   ##
==========================================
- Coverage   19.82%   18.45%   -1.38%     
==========================================
  Files         202      213      +11     
  Lines       27685    30216    +2531     
==========================================
+ Hits         5489     5575      +86     
- Misses      21175    23609    +2434     
- Partials     1021     1032      +11

Impacted Files	Coverage Δ
x/gamm/pool-models/stableswap/amm.go	`43.28% <0.00%> (-12.49%)`	⬇️
x/gamm/pool-models/stableswap/pool.go	`0.00% <0.00%> (ø)`
x/lockup/client/cli/query.go	`51.99% <0.00%> (-3.00%)`	⬇️
x/lockup/keeper/grpc_query.go	`50.40% <0.00%> (-4.47%)`	⬇️
x/lockup/keeper/store.go	`84.95% <0.00%> (-2.32%)`	⬇️
x/lockup/keeper/iterator.go	`82.92% <33.33%> (-2.08%)`	⬇️
x/lockup/keeper/lock.go	`56.20% <65.21%> (+0.88%)`	⬆️
x/lockup/keeper/msg_server.go	`81.44% <76.47%> (-1.06%)`	⬇️
x/gamm/pool-models/balancer/amm.go	`25.12% <87.50%> (+7.53%)`	⬆️
x/gamm/client/cli/tx.go	`59.50% <100.00%> (-0.69%)`	⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9b9804...dba0383. Read the comment docs.

x/lockup/keeper/lock.go

stackman27 · 2022-04-20T20:41:49Z

Closes #164 ?

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

…ration

…ockup-increase-duration

…-labs/osmosis into mconcat/lockup-increase-duration

alexanderbez · 2022-04-27T22:40:31Z

@mattverse wanna give this another look?

mattverse

Don't want to block this PR for so long being picky 😅

Went through final review, should be good to merge after fix!

x/lockup/keeper/msg_server.go

x/lockup/keeper/lock_test.go

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

mattverse

🌮

proto/osmosis/lockup/tx.proto

ValarDragon · 2022-05-04T01:17:44Z

x/lockup/keeper/lock.go

+	k.hooks.OnTokenUnlocked(ctx, lock.OwnerAddress(), lock.ID, lock.Coins, oldLock.Duration, lock.EndTime)
+	k.hooks.OnTokenLocked(ctx, lock.OwnerAddress(), lock.ID, lock.Coins, lock.Duration, lock.EndTime)


Lets re-assess these hook calls in a follow-up issue

ValarDragon · 2022-05-04T03:42:10Z

x/lockup/keeper/msg_server_test.go

@@ -261,3 +262,78 @@ func (suite *KeeperTestSuite) TestMsgBeginUnlockingAll() {
 		}
 	}
 }
+
+func (suite *KeeperTestSuite) TestMsgEditLockup() {


Great job on this test!

ValarDragon

Nice work on this!

add EditLockup

143abba

remove gw file fdsa

github-actions bot added the C:x/lockup label Apr 14, 2022

mconcat added 2 commits April 14, 2022 23:44

fix syntax

e60fb3b

gofmt

b202571

fix

520f40b

alexanderbez requested review from ValarDragon and mattverse April 16, 2022 11:11

alexanderbez reviewed Apr 16, 2022

View reviewed changes

mattverse requested changes Apr 18, 2022

View reviewed changes

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

mconcat and others added 3 commits April 18, 2022 23:15

Apply suggestions from code review

ba815a5

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

EditLockup -> ExtendLockup, use Wrapf

54eff0f

fix test

a79cbda

add duration check on MsgExtendLockup

f61fed4

mattverse mentioned this pull request Apr 18, 2022

x/lock: Investigate setLockAndResetLockRefs #1275

Closed

alexanderbez reviewed Apr 18, 2022

View reviewed changes

mconcat and others added 5 commits April 25, 2022 21:52

Apply suggestions from code review

5c45211

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Update x/lockup/keeper/lock.go

b6e31c9

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

fix msg validation

5999bd2

Merge remote-tracking branch 'origin' into mconcat/lockup-increase-du…

412a253

…ration

protodoc for MsgExtendLockup

32b2a27

alexanderbez approved these changes Apr 25, 2022

View reviewed changes

mconcat added 4 commits April 28, 2022 03:01

Merge branch 'main' of github.com:osmosis-labs/osmosis into mconcat/l…

368008f

…ockup-increase-duration

fix lock lint

5d9d276

Merge branch 'mconcat/lockup-increase-duration' of github.com:osmosis…

a8e5442

…-labs/osmosis into mconcat/lockup-increase-duration

typo

5d1c61d

Merge branch 'main' into mconcat/lockup-increase-duration

9415a4a

alexanderbez approved these changes Apr 27, 2022

View reviewed changes

mattverse reviewed Apr 28, 2022

View reviewed changes

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

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

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

mconcat and others added 4 commits May 3, 2022 00:07

Apply suggestions from code review

78c555c

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

add hook call in extendlock

0ebe1ef

fix test

dba0383

Merge branch 'main' into mconcat/lockup-increase-duration

9b9e63e

mattverse approved these changes May 3, 2022

View reviewed changes

ValarDragon reviewed May 3, 2022

View reviewed changes

proto/osmosis/lockup/tx.proto Show resolved Hide resolved

Merge branch 'main' into mconcat/lockup-increase-duration

2d334bc

alexanderbez requested a review from a team May 3, 2022 21:33

ValarDragon reviewed May 4, 2022

View reviewed changes

ValarDragon approved these changes May 4, 2022

View reviewed changes

ValarDragon merged commit 6e1f93a into main May 4, 2022

ValarDragon deleted the mconcat/lockup-increase-duration branch May 4, 2022 03:42

ValarDragon mentioned this pull request May 4, 2022

Investigate ExtendLockup hooks #1404

Closed

github-actions bot mentioned this pull request Apr 15, 2024

Broken Links Detected #8046

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lockup duration edit method #1253

Add lockup duration edit method #1253

mconcat commented Apr 14, 2022 •

edited by alexanderbez

Loading

ValarDragon commented Apr 14, 2022

mconcat commented Apr 14, 2022

ValarDragon commented Apr 15, 2022

mattverse left a comment

codecov-commenter commented Apr 18, 2022 •

edited

Loading

stackman27 commented Apr 20, 2022

alexanderbez commented Apr 27, 2022

mattverse left a comment

mattverse left a comment

ValarDragon May 4, 2022

ValarDragon May 4, 2022

ValarDragon left a comment

		k.hooks.OnTokenUnlocked(ctx, lock.OwnerAddress(), lock.ID, lock.Coins, oldLock.Duration, lock.EndTime)
		k.hooks.OnTokenLocked(ctx, lock.OwnerAddress(), lock.ID, lock.Coins, lock.Duration, lock.EndTime)

Add lockup duration edit method #1253

Add lockup duration edit method #1253

Conversation

mconcat commented Apr 14, 2022 • edited by alexanderbez Loading

Description

ValarDragon commented Apr 14, 2022

mconcat commented Apr 14, 2022

ValarDragon commented Apr 15, 2022

mattverse left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 18, 2022 • edited Loading

Codecov Report

stackman27 commented Apr 20, 2022

alexanderbez commented Apr 27, 2022

mattverse left a comment

Choose a reason for hiding this comment

mattverse left a comment

Choose a reason for hiding this comment

ValarDragon May 4, 2022

Choose a reason for hiding this comment

ValarDragon May 4, 2022

Choose a reason for hiding this comment

ValarDragon left a comment

Choose a reason for hiding this comment

mconcat commented Apr 14, 2022 •

edited by alexanderbez

Loading

codecov-commenter commented Apr 18, 2022 •

edited

Loading