Skip to content
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

Issue 1525: sub-section: combine superfluid query and staking query on querying delegation by delegator #1539

Merged
merged 16 commits into from
Jun 30, 2022

Conversation

nghuyenthevinh2000
Copy link
Member

@nghuyenthevinh2000 nghuyenthevinh2000 commented May 19, 2022

Closes: #1525

What is the purpose of the change

Combining superfluid query and staking query on querying delegation by delegator

Brief Changelog

  • add total-delegation-by-delegator to superfluid cli. It will query both superfluid delegation and normal delegation of a delegator.
  • add proto rpc to superfluid/query.proto. It will return following information:
message QueryTotalDelegationByDelegatorResponse {
  repeated SuperfluidDelegationRecord superfluid_delegation_records = 1
      [ (gogoproto.nullable) = false ];

  repeated cosmos.staking.v1beta1.DelegationResponse delegation_response = 2 [
    (gogoproto.nullable) = false
  ];
  repeated cosmos.base.v1beta1.Coin total_delegated_coins = 3 [
    (gogoproto.nullable) = false,
    (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins"
  ];
  cosmos.base.v1beta1.Coin total_equivalent_staked_amount = 4 [
    (gogoproto.nullable) = false,
    (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coin"
  ];
} 

the total_equivalent_staked_amount one is idea and code from @hieuvubk

  • add logic TotalDelegationByDelegator() in combined_grpc_query.go (a new file set as foundation for later combined query between staking and superfluid delegation)

Testing and Verifying

Testing is as following:

  • create 2 mock account
  • 2 mock account will superfluid delegate 1000000 pool/gamm/1 and 1000000 pool/gamm/2, and delegate 18 uosmo to 2 validators. (9 osmo each)
  • the expected result would be to check if total_delegated_coins returns correcly 3 delegation of a delegator, returns correctly 2 delegations in delegation_response, returns correctly 2 delegations in superfluid_delegation_records, the amount of total_equivalent_staked_amount has to be correctly the total osmo equivalent of all delegations.

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?

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2022

Codecov Report

Merging #1539 (7642977) into main (725249f) will decrease coverage by 0.04%.
The diff coverage is 6.70%.

@@            Coverage Diff             @@
##             main    #1539      +/-   ##
==========================================
- Coverage   19.57%   19.53%   -0.05%     
==========================================
  Files         236      243       +7     
  Lines       31639    32458     +819     
==========================================
+ Hits         6194     6341     +147     
- Misses      24310    24958     +648     
- Partials     1135     1159      +24     
Impacted Files Coverage Δ
x/epochs/types/query.pb.gw.go 0.00% <0.00%> (ø)
x/gamm/types/query.pb.gw.go 0.00% <0.00%> (ø)
x/incentives/types/query.pb.gw.go 0.00% <0.00%> (ø)
x/mint/types/query.pb.gw.go 0.00% <0.00%> (ø)
x/pool-incentives/types/query.pb.gw.go 0.00% <0.00%> (ø)
x/superfluid/client/cli/query.go 0.00% <0.00%> (ø)
x/superfluid/keeper/combined_grpc_query.go 0.00% <0.00%> (ø)
x/superfluid/keeper/keeper.go 100.00% <ø> (ø)
x/tokenfactory/types/query.pb.gw.go 0.00% <0.00%> (ø)
x/superfluid/keeper/grpc_query.go 49.15% <85.71%> (+2.30%) ⬆️
... and 34 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 725249f...7642977. Read the comment docs.

Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I went through and did a brief review, here's what I think:

SuperfluidDelegationsByDelegator currently returns all open positions for the superfluid lock: meaning it doesn't return how much osmo equivilent is staked for the user's gamm share, but returns the amount of gamm share itself. That being said, I personally think adding how much osmo equivilent is staked via superfluid to this query would cause confusion if we directly add this to the coins. IMO would be possibly better if we created a new field for this in the struct.

In the same time, I also think creating a separate query for this would be useful as well!

Otherwise, I think the logic itself is LGTM! This is awesome! tysm

@hieuvubk
Copy link
Contributor

#1632
can you take a look at this pr @mattverse

@ValarDragon
Copy link
Member

Why is this changing every proto

@hieuvubk
Copy link
Contributor

hieuvubk commented Jun 2, 2022

Why is this changing every proto

#1632
can u review this pr

@nghuyenthevinh2000
Copy link
Member Author

nghuyenthevinh2000 commented Jun 3, 2022

I think that issue #1525 has a broad scope and should have several prs to fulfill that issue.

This pr scope is to combine superfluid query and staking query on querying delegation by delegator.

This pr would combine with @hieuvubk idea and code of osmo equivalent in #1632.

So, I think that this one is ready for review.

@nghuyenthevinh2000 nghuyenthevinh2000 marked this pull request as ready for review June 3, 2022 06:36
@nghuyenthevinh2000 nghuyenthevinh2000 requested a review from a team June 3, 2022 06:36
@nghuyenthevinh2000 nghuyenthevinh2000 changed the title Issue 1525 Issue 1525: sub-section: combine superfluid query and staking query on querying delegation by delegator Jun 3, 2022
@nghuyenthevinh2000
Copy link
Member Author

Why is this changing every proto

I have changed it so as to correctly change relevant proto files

Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments!

Can we also revert changing go.mod and go.sum? Thanks!

proto/osmosis/superfluid/query.proto Show resolved Hide resolved
x/superfluid/client/cli/query.go Outdated Show resolved Hide resolved
x/superfluid/client/cli/query.go Outdated Show resolved Hide resolved
x/superfluid/keeper/combined_grpc_query.go Outdated Show resolved Hide resolved
x/superfluid/keeper/grpc_query_test.go Outdated Show resolved Hide resolved
x/superfluid/keeper/grpc_query_test.go Outdated Show resolved Hide resolved
x/superfluid/keeper/combined_grpc_query.go Outdated Show resolved Hide resolved
x/superfluid/keeper/combined_grpc_query.go Outdated Show resolved Hide resolved
x/superfluid/keeper/combined_grpc_query.go Outdated Show resolved Hide resolved
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM!

Can you just add a changelog entry?

@github-actions github-actions bot added C:docs Improvements or additions to documentation T:build labels Jun 28, 2022
@nghuyenthevinh2000
Copy link
Member Author

This LGTM!

Can you just add a changelog entry?

added, hope I add to correct place.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

This was a big feature! Thanks for this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:docs Improvements or additions to documentation C:x/superfluid T:build
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support superfluid staking queries
6 participants