Feat: CLI account-locked-duration #1292

CmpHDL · 2022-04-19T00:51:18Z

Closes: #756

Description

Adds CLI command to query pool lockups with given duration. The jitter issue outlined in #756 no longer applies due to +2 weeks (longest lockup time) passing since v7 has been out.

Example Usage:

Query account locked records with a specific duration.

Example:
$ osmosisd query lockup account-locked-duration <address> <duration>

Usage:
  osmosisd query lockup account-locked-duration <address> <duration> [flags]

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

CmpHDL · 2022-04-19T01:40:24Z

This was based off of v7.0.2, I can re-base this off of main if needed.

…equest, and Response

…eratorDuration method.

ValarDragon · 2022-04-19T14:04:00Z

Please do rebase the change off of main

codecov-commenter · 2022-04-19T14:19:50Z

Codecov Report

Merging #1292 (77891d4) into main (f56fbe5) will decrease coverage by 0.07%.
The diff coverage is 3.26%.

@@            Coverage Diff             @@
##             main    #1292      +/-   ##
==========================================
- Coverage   19.82%   19.74%   -0.08%     
==========================================
  Files         202      202              
  Lines       27685    27759      +74     
==========================================
- Hits         5489     5482       -7     
- Misses      21175    21256      +81     
  Partials     1021     1021

Impacted Files	Coverage Δ
x/gamm/pool-models/balancer/amm.go	`17.58% <ø> (ø)`
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/gamm/client/cli/tx.go	`59.50% <100.00%> (-0.69%)`	⬇️

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 a56fe51...77891d4. Read the comment docs.

CmpHDL · 2022-04-19T20:40:38Z

@ValarDragon I had rebased off main after that comment starting with commit 93fd8c3, were you wanting to remerge main based on new commits since then? Also I refixed the typo that somehow got reversed during the rebase so a rerun of workflows should satisfy the linter.

alexanderbez · 2022-04-20T14:18:52Z

proto/osmosis/lockup/query.proto

+  rpc AccountLockedDuration(AccountLockedDurationRequest)
+      returns (AccountLockedDurationResponse) {
+    option (google.api.http).get =
+        "/osmosis/lockup/v1beta1/account_locked_duration/{owner}";


Since AccountLockedDurationRequest defines the query params, is there a point in having {owner} as part of the resource URL?

Can you elaborate on this? AccountLockedPastTimeDenom, AccountLockedPastTimeNotUnlockingOnlyRequest, and most of the locked RPC commands have the {owner} within the resource URL, I had assumed it was for the address parameter and maintained practice.

@alexanderbez I'm not very familiar with this RPC library - does it make sense for us to create an issue and remove potentially redundant {owner}?

These urls are mostly used in front end, but we definitely don't need to have them both in the request url and the request itself. However, I don't know if it's worth changing considering the fact that all the urls and queries in the frontend would have to be updated along as well

proto/osmosis/lockup/query.proto

x/lockup/client/cli/query.go

x/lockup/keeper/iterator.go

Co-authored-by: Roman <[email protected]>

CmpHDL · 2022-04-20T18:35:06Z

Thanks for this PR!

I might have misunderstood issue #756, but seems to me that the direction this PR points to vs the original issue mentioned in the issue seems different.

How I understood issue #756 was that we can't query total amount of specific denom that's over specific amount of duration, since the query account-locked-longer-duration-denom returns all lockups separately instead of adding up the what's in individual lockups.

This PR, however seems to have the same problem, seems like it's closer to adding a query to query all denoms instead of specific denom over specified time. (I still think this query is superuseful, just wanted to confirm my understanding on the existing issue and the current PR submitted!)

The ticket changed a little due to jitter no longer being an issue and yes I planned todo the same command but for denoms after this one. Rather than do both of them at once I wanted to see the process etc for making a change and what kinds of feedback you would have, and better understand things as a whole, rather than making the same mistakes twice and having to fix them twice in separate PRs.

tl;dr
Once this goes through I will be making the command you're talking about with denom as an additional parameter, this one was easier so decided to give it a go right before its brother which includes denom.

…s query.

mattverse

Seems that there's linting issues in query.go thats making the checks fail.

LGTM once that's been fixed!

CmpHDL · 2022-04-25T13:35:26Z

Thanks @mattverse Will get this fixed today, oddly enough it doesn't seem to be related to this ticket.

CmpHDL · 2022-04-25T13:42:41Z

@mattverse Ended up fixing it now, query.go should pass, looks like a cat had walked along mid commit.

CHANGELOG.md

x/lockup/client/cli/query.go

CmpHDL · 2022-04-25T21:00:42Z

@alexanderbez I made the changes you requested.

p0mvn

Please address the remaining minor comments but otherwise LGTM. Great job!

x/lockup/client/cli/query.go

p0mvn · 2022-04-26T02:13:55Z

proto/osmosis/lockup/query.proto

+  rpc AccountLockedDuration(AccountLockedDurationRequest)
+      returns (AccountLockedDurationResponse) {
+    option (google.api.http).get =
+        "/osmosis/lockup/v1beta1/account_locked_duration/{owner}";


@alexanderbez I'm not very familiar with this RPC library - does it make sense for us to create an issue and remove potentially redundant {owner}?

x/lockup/keeper/iterator.go

CmpHDL · 2022-04-26T12:21:15Z

@p0mvn Requested changes done.

github-actions bot added C:CLI C:x/lockup C:x/superfluid labels Apr 19, 2022

CmpHDL changed the title ~~7.0.2 t756~~ Feat: CLI account-locked-duration Apr 19, 2022

ValarDragon and others added 11 commits April 18, 2022 22:01

Add panic guard, bug fix

7d39255

Add AccountLockedDuration RPC service

7323906

Add AccountLockedDuration Request & Response Messages

85f1c3c

Generated protofiles for x/lockup, added AccountLockedDuration RPC, R…

c728281

…equest, and Response

Update CLI Request & Response for account-locked-duration

38d1180

Add account_locked_duration to querier types

3fbb784

Add comments & Update lockup keeper iterator to include AccountLockIt…

3c6c6d1

…eratorDuration method.

Add GetAccountLockedDuration to lockup store and use new iterator

4728be4

Fix method comment namiing

5a4d7a1

Add missing AccountLockedDuration method to grpc_query.go/

e8bf393

Add documentation for rpc AccountLockedDuration

93fd8c3

CmpHDL force-pushed the 7.0.2-T756 branch from 4994c6f to 93fd8c3 Compare April 19, 2022 02:10

github-actions bot removed the C:x/superfluid label Apr 19, 2022

CmpHDL added 2 commits April 18, 2022 22:31

Fixed weird rebase error.

d09b708

Refactored Keeper into Querier

7a830c6

CmpHDL force-pushed the 7.0.2-T756 branch from fbdfcd4 to 7a830c6 Compare April 19, 2022 02:33

Readd GetCmdAccountLockedDuration to QueryTree

5a15cd5

Fix occurred mispelling to satisfy linter

df3b68f

alexanderbez requested review from alexanderbez, mattverse and p0mvn April 20, 2022 14:16

alexanderbez reviewed Apr 20, 2022

View reviewed changes

CmpHDL and others added 5 commits April 20, 2022 14:18

Update x/lockup/keeper/grpc_query.go

066727f

Co-authored-by: Roman <[email protected]>

Update x/lockup/keeper/iterator.go

9bd7ff6

Co-authored-by: Roman <[email protected]>

Update x/lockup/keeper/store.go

dfe4272

Co-authored-by: Roman <[email protected]>

Update x/lockup/keeper/store.go

50b1acc

Co-authored-by: Roman <[email protected]>

Remove TODOs.

5d31a26

CmpHDL added 3 commits April 20, 2022 14:43

Add PR to changelog Unreleased

e92923d

Fix initialization order of interatorDuration.

fe85f63

Add example address to long description of GetCmdAccountUnlockingCoin…

c08a216

…s query.

CmpHDL requested review from p0mvn, mattverse and alexanderbez April 23, 2022 13:34

mattverse self-assigned this Apr 23, 2022

mattverse approved these changes Apr 25, 2022

View reviewed changes

Fixed Bech32 address in long description and function call.

44350e1

alexanderbez reviewed Apr 25, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

x/lockup/client/cli/query.go Outdated Show resolved Hide resolved

CmpHDL added 2 commits April 25, 2022 16:57

Swap Long for Example

da8ce5f

Update CHANGELOG

2678e1b

p0mvn approved these changes Apr 26, 2022

View reviewed changes

CmpHDL added 3 commits April 26, 2022 08:17

Add example for GetCmdAccountLockedDuration

d635e43

Remove example from wrong function

ad523ac

Fix comment for AccountLockIteratorShorterThanDuration

ce4a764

CmpHDL requested a review from alexanderbez April 26, 2022 12:21

Merge branch 'main' into 7.0.2-T756

77891d4

czarcas7ic merged commit ad7d5de into osmosis-labs:main Apr 27, 2022

github-actions bot mentioned this pull request Mar 1, 2024

Broken Links Detected #7641

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: CLI account-locked-duration #1292

Feat: CLI account-locked-duration #1292

CmpHDL commented Apr 19, 2022 •

edited

Loading

CmpHDL commented Apr 19, 2022

ValarDragon commented Apr 19, 2022

codecov-commenter commented Apr 19, 2022 •

edited

Loading

CmpHDL commented Apr 19, 2022

alexanderbez Apr 20, 2022

CmpHDL Apr 20, 2022

p0mvn Apr 26, 2022

mattverse Apr 26, 2022 •

edited

Loading

CmpHDL commented Apr 20, 2022

mattverse left a comment •

edited

Loading

CmpHDL commented Apr 25, 2022

CmpHDL commented Apr 25, 2022

CmpHDL commented Apr 25, 2022 •

edited

Loading

p0mvn left a comment

p0mvn Apr 26, 2022

CmpHDL commented Apr 26, 2022

Feat: CLI account-locked-duration #1292

Feat: CLI account-locked-duration #1292

Conversation

CmpHDL commented Apr 19, 2022 • edited Loading

Description

CmpHDL commented Apr 19, 2022

ValarDragon commented Apr 19, 2022

codecov-commenter commented Apr 19, 2022 • edited Loading

Codecov Report

CmpHDL commented Apr 19, 2022

alexanderbez Apr 20, 2022

Choose a reason for hiding this comment

CmpHDL Apr 20, 2022

Choose a reason for hiding this comment

p0mvn Apr 26, 2022

Choose a reason for hiding this comment

mattverse Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

CmpHDL commented Apr 20, 2022

mattverse left a comment • edited Loading

Choose a reason for hiding this comment

CmpHDL commented Apr 25, 2022

CmpHDL commented Apr 25, 2022

CmpHDL commented Apr 25, 2022 • edited Loading

p0mvn left a comment

Choose a reason for hiding this comment

p0mvn Apr 26, 2022

Choose a reason for hiding this comment

CmpHDL commented Apr 26, 2022

CmpHDL commented Apr 19, 2022 •

edited

Loading

codecov-commenter commented Apr 19, 2022 •

edited

Loading

mattverse Apr 26, 2022 •

edited

Loading

mattverse left a comment •

edited

Loading

CmpHDL commented Apr 25, 2022 •

edited

Loading