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

add governance support for superfluid #1191

Merged
merged 25 commits into from
May 17, 2022
Merged

Conversation

mconcat
Copy link
Collaborator

@mconcat mconcat commented Apr 4, 2022

Closes: #1161

stil fixing test

Description


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

@mconcat mconcat marked this pull request as ready for review April 12, 2022 17:08
@mconcat
Copy link
Collaborator Author

mconcat commented Apr 12, 2022

The superfluid governance itself is working, but an irrelevant test is failing. Trying to figure out.

x/superfluid/keeper/stake.go Show resolved Hide resolved
x/superfluid/keeper/stake.go Outdated Show resolved Hide resolved
x/superfluid/keeper/stake.go Show resolved Hide resolved
@mconcat
Copy link
Collaborator Author

mconcat commented Apr 18, 2022

The test on keeper/stake_test.go is failing, which is not touched by any edit in this PR. Please let me know if someone knows why this happens!

EDIT: nvm fixed it

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

I made some suggestions to the panic strings. Do you think you can apply those suggestions to all the panic strings?

Also, this has me thinking...should we be panicing here? My gut tells me we should instead return an error and bubble it up to the caller.

/cc @ValarDragon

x/superfluid/keeper/stake.go Outdated Show resolved Hide resolved
x/superfluid/keeper/stake.go Outdated Show resolved Hide resolved
x/superfluid/keeper/stake.go 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.

Are there specific reasons IterateBondedValidatorsByPower, TotalBondedTokens, IterateDelegations was implemented amongst other staking methods?

Also questioned how are these to be implemented for query(would our sdk fork be implementing these methods?)

x/superfluid/keeper/stake.go Outdated Show resolved Hide resolved
x/superfluid/keeper/stake_test.go Outdated Show resolved Hide resolved
@alexanderbez
Copy link
Contributor

Let's get CI/build passing and we can get this merged.

@ValarDragon
Copy link
Member

Are there specific reasons IterateBondedValidatorsByPower, TotalBondedTokens, IterateDelegations was implemented amongst other staking methods?

Its for interface satisfaction. (Satisfying what the governance keeper expects from a staking keeper)

Re: Queries, we should separately figure out either getting staking queries to show superfluid delegations, or superfluid queries to have conjoined queries.

@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2022

Codecov Report

Merging #1191 (c3e4d6d) into main (f149135) will increase coverage by 0.07%.
The diff coverage is 31.21%.

@@            Coverage Diff             @@
##             main    #1191      +/-   ##
==========================================
+ Coverage   19.50%   19.57%   +0.07%     
==========================================
  Files         231      236       +5     
  Lines       31527    31639     +112     
==========================================
+ Hits         6150     6194      +44     
- Misses      24251    24310      +59     
- Partials     1126     1135       +9     
Impacted Files Coverage Δ
osmoutils/parse.go 0.00% <0.00%> (ø)
x/gamm/keeper/msg_server.go 2.81% <ø> (-1.24%) ⬇️
x/gamm/module.go 64.10% <ø> (-2.57%) ⬇️
x/gamm/pool-models/balancer/amm.go 25.37% <0.00%> (+3.76%) ⬆️
x/gamm/pool-models/stableswap/amm.go 44.20% <0.00%> (-7.06%) ⬇️
x/gamm/pool-models/stableswap/pool.go 0.00% <0.00%> (ø)
x/pool-incentives/client/cli/tx.go 0.00% <0.00%> (ø)
x/superfluid/keeper/stake.go 61.05% <50.00%> (-2.32%) ⬇️
osmoutils/binary_search.go 79.48% <79.48%> (ø)
osmoutils/cache_ctx.go 0.00% <0.00%> (ø)
... and 2 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 d3af599...c3e4d6d. Read the comment docs.

@p0mvn
Copy link
Member

p0mvn commented May 4, 2022

Hi @mconcat . Just noticed that there haven't been any updates for a few days.

What is the status of this?

If this is still in progress, let's mark it as a draft, please

@mconcat
Copy link
Collaborator Author

mconcat commented May 5, 2022

I added test for the native coins - I think this is good to merge!

@mconcat mconcat requested a review from a team May 5, 2022 03:39
x/superfluid/keeper/stake.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.

LGTM, thanks for the updated test! I think its good to merge once bez and my comments addressed

@mattverse mattverse mentioned this pull request May 17, 2022
14 tasks
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.

@ValarDragon
Copy link
Member

ty for code review fixes matt!

@ValarDragon ValarDragon merged commit e3f09f3 into main May 17, 2022
@ValarDragon ValarDragon deleted the mconcat/superfluid-governance branch May 17, 2022 16:44
mergify bot pushed a commit that referenced this pull request May 21, 2022
Closes: #XXX

## What is the purpose of the change

@czarcas7ic and I ran into issues manually testing #1191 . We both reproduces this test case: #1543 (comment)

Upon further investigation, it was found that staking keeper was never replaced with superfluid keeper in the gov module so that the new logic could not be used for calculating tally and overriding validator votes by SF stakers.

We need to e2e test this case in a future PR: #1556

## Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

## 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 applicable
mergify bot pushed a commit that referenced this pull request May 21, 2022
Closes: #XXX

## What is the purpose of the change

@czarcas7ic and I ran into issues manually testing #1191 . We both reproduces this test case: https://github.com/osmosis-labs/osmosis/discussions/1543#discussioncomment-2786650

Upon further investigation, it was found that staking keeper was never replaced with superfluid keeper in the gov module so that the new logic could not be used for calculating tally and overriding validator votes by SF stakers.

We need to e2e test this case in a future PR: #1556

## Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

## 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 applicable

(cherry picked from commit 8aaa84b)
p0mvn added a commit that referenced this pull request May 21, 2022
Closes: #XXX

## What is the purpose of the change

@czarcas7ic and I ran into issues manually testing #1191 . We both reproduces this test case: https://github.com/osmosis-labs/osmosis/discussions/1543#discussioncomment-2786650

Upon further investigation, it was found that staking keeper was never replaced with superfluid keeper in the gov module so that the new logic could not be used for calculating tally and overriding validator votes by SF stakers.

We need to e2e test this case in a future PR: #1556

## Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

## 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 applicable

(cherry picked from commit 8aaa84b)

Co-authored-by: Roman <roman@osmosis.team>
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Make superfluid staking stakers get direct governance votes
6 participants