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

refactor: add PokeTokenWeights to Pool interface #1232

Merged
merged 8 commits into from
Apr 12, 2022
Merged

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Apr 11, 2022

Closes: #1213

Description

  • Add PokePool to the Pool interface
  • Rename GetPool -> GetPoolAndPoke
  • Rename GetPools -> GetPoolsAndPoke

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

@alexanderbez alexanderbez added the C:x/gamm Changes, features and bugs related to the gamm module. label Apr 11, 2022
//
// 3. t > start_time + duration: w(t) = target_pool_weights
switch {
case blockTime.Before(params.StartTime) || params.StartTime.Equal(blockTime):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reviewer: no logical changes, just switched the existing branching code to be switch-based instead of if/elses.

Copy link
Member

Choose a reason for hiding this comment

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

should we generally aim to prefer the code base to be switch based instead of if/elses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, definitely. A single if/else is fine and OK. Once you have an if/if else/.../else block, then it becomes hard to read and a switch is much cleaner.

@alexanderbez
Copy link
Contributor Author

alexanderbez commented Apr 11, 2022

@ValarDragon we actually don't have any keeper-based tests for this, so even with this change, we wouldn't catch anything I think?

Also, more I think about it, idk how I feel about a Get* function modifying state...can we maybe introduce a new API as well -- GetUpdatedWeightedPool(s)?

@alexanderbez alexanderbez marked this pull request as ready for review April 11, 2022 14:14
x/gamm/types/pool.go Outdated Show resolved Hide resolved
@ValarDragon
Copy link
Member

Agreed with the concern over GetPool running this function. Perhaps we can rename to GetPoolAndPoke ?

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!
A general question I have would be for none balancer pools, how/what would PokeTokenWeights be implemented?

//
// 3. t > start_time + duration: w(t) = target_pool_weights
switch {
case blockTime.Before(params.StartTime) || params.StartTime.Equal(blockTime):
Copy link
Member

Choose a reason for hiding this comment

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

should we generally aim to prefer the code base to be switch based instead of if/elses?

@alexanderbez alexanderbez marked this pull request as draft April 11, 2022 16:26
@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2022

Codecov Report

Merging #1232 (a91c0ab) into main (20cfa05) will increase coverage by 0.01%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##             main    #1232      +/-   ##
==========================================
+ Coverage   20.96%   20.98%   +0.01%     
==========================================
  Files         196      196              
  Lines       25337    25340       +3     
==========================================
+ Hits         5312     5317       +5     
+ Misses      19066    19065       -1     
+ Partials      959      958       -1     
Impacted Files Coverage Δ
x/gamm/types/pool.go 0.00% <ø> (ø)
x/gamm/keeper/grpc_query.go 42.20% <83.33%> (ø)
x/gamm/genesis.go 68.00% <100.00%> (ø)
x/gamm/keeper/invariants.go 29.41% <100.00%> (ø)
x/gamm/keeper/pool.go 66.10% <100.00%> (+1.18%) ⬆️
x/gamm/keeper/pool_service.go 56.61% <100.00%> (ø)
x/gamm/pool-models/balancer/balancer_pool.go 63.09% <100.00%> (+1.02%) ⬆️
x/superfluid/keeper/epoch.go 67.92% <100.00%> (ø)

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 20cfa05...a91c0ab. Read the comment docs.

@alexanderbez alexanderbez marked this pull request as ready for review April 12, 2022 09:10
@@ -359,7 +359,7 @@ func TestBalancerPoolPokeTokenWeights(t *testing.T) {
defaultStartTime := time.Unix(1618703511, 0)
defaultStartTimeUnix := defaultStartTime.Unix()
defaultDuration := 100 * time.Second
floatGuaranteedPrecison := float64(GuaranteedWeightPrecision)
floatGuaranteedPrecision := float64(GuaranteedWeightPrecision)
Copy link
Member

Choose a reason for hiding this comment

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

lol, ty for the fix

@ValarDragon ValarDragon merged commit b6d342a into main Apr 12, 2022
@ValarDragon ValarDragon deleted the bez/1213-poke-pool branch April 12, 2022 15:30
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] Add PokePool function to AMM interface
4 participants