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

Scaling factor governor #1499

Merged
merged 16 commits into from
May 19, 2022
Merged

Scaling factor governor #1499

merged 16 commits into from
May 19, 2022

Conversation

vuong177
Copy link
Contributor

@vuong177 vuong177 commented May 16, 2022

Closes: #1460

What is the purpose of the change

Add a description of the overall background and high level changes that this PR introduces

(E.g.: This pull request improves documation of area A by adding ....

Brief Changelog

(for example:)

  • The metadata is stored in the blob store on job creation time as a persistent artifact
  • Deployments RPC transmits only the blob storage reference
  • Daemons retrieve the RPC data from the blob cache

Testing and Verifying

(Please pick one of the following options)

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

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added unit test that validates ...
  • Added integration tests for end-to-end deployment with ...
  • Extended integration test for ...
  • Manually verified the change by ...

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@vuong177 vuong177 requested a review from ValarDragon May 16, 2022 16:01
@github-actions github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label May 16, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 16, 2022

Codecov Report

Merging #1499 (bd83d2d) into main (bcd5942) will decrease coverage by 0.20%.
The diff coverage is 0.58%.

@@            Coverage Diff             @@
##             main    #1499      +/-   ##
==========================================
- Coverage   19.66%   19.46%   -0.21%     
==========================================
  Files         241      241              
  Lines       31837    32181     +344     
==========================================
+ Hits         6261     6263       +2     
- Misses      24424    24766     +342     
  Partials     1152     1152              
Impacted Files Coverage Δ
x/gamm/keeper/msg_server.go 2.81% <ø> (ø)
x/gamm/keeper/pool.go 53.42% <0.00%> (-12.68%) ⬇️
x/gamm/pool-models/stableswap/msgs.go 32.07% <0.00%> (-15.15%) ⬇️
.../gamm/pool-models/stableswap/stableswap_pool.pb.go 0.54% <0.00%> (-0.04%) ⬇️
x/gamm/pool-models/stableswap/tx.pb.go 0.74% <0.71%> (-0.03%) ⬇️

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 bcd5942...bd83d2d. Read the comment docs.

@vuong177 vuong177 marked this pull request as ready for review May 18, 2022 10:56
@vuong177 vuong177 requested a review from a team May 18, 2022 10:56
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.

Awesome, thanks so much for adding this!

Want to just add a test that ensures the factors do get updated in state, then we can merge it in?

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
@vuong177
Copy link
Contributor Author

vuong177 commented May 18, 2022

Awesome, thanks so much for adding this!

Want to just add a test that ensures the factors do get updated in state, then we can merge it in?

yep ! I'll add that test. Thanks for reviewing !

@vuong177
Copy link
Contributor Author

I added test but commented it out because stableswap still disable

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.

Thanks for this PR! Nit for the ones I left reviews on the commented ones, seems that we still need to add codec.go before merging though

x/gamm/keeper/msg_server.go Outdated Show resolved Hide resolved
x/gamm/keeper/msg_server.go Outdated Show resolved Hide resolved
x/gamm/pool-models/stableswap/msgs.go 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

@vuong177 vuong177 merged commit 82e58f8 into osmosis-labs:main May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/gamm Changes, features and bugs related to the gamm module.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[x/gamm][StableSwap - Post V8] Add stableswap scaling factor governor
4 participants