chore: v17 upgrade handler pool create/link/sfs #5895
Conversation
|
Just need to implement new e2e logic for all the new pools getting migrated. Halfway done with a solution but its proving to be slightly trickier than I originally thought it would be. |
| @@ -17,3 +18,204 @@ var Upgrade = upgrades.Upgrade{ | |||
| Deleted: []string{}, | |||
| }, | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
This whole file should be verified against mainnet by the reviewer
There was a problem hiding this comment.
The only thing that needs to be verified is the pool numbers now, since the other values are auto populated at time of execution
There was a problem hiding this comment.
checked the assetPairs to migrate and no eth bridge tokens found + all fees are correct + all osmo quoted!
ps: i'm surprised we're not migrating osmo/atom pool
There was a problem hiding this comment.
I think we are saving the two highest value pools for last. Plan was to work our way down from osmo/dai and then return to top.
Co-authored-by: Sishir Giri <[email protected]>
github-actions
bot
added
the
C:x/gamm
There was a problem hiding this comment.
this LGTM! checked all the Asset pairs manually using app.osmosis.zone & https://info.osmosis.zone
| } else { | ||
| denom1 = coin.Denom | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: wouldn't this logic be cleaner with just two if statements checking each denom?
There was a problem hiding this comment.
Simplified here bb111c0
|
|
||
| // If the spread factor is not manually set above, set it to the the same value as the pool's spread factor. | ||
| if assetPair.SpreadFactor.IsNil() { | ||
| AssetPairs[i].SpreadFactor = pool.GetSpreadFactor(ctx) |
There was a problem hiding this comment.
Just to clarify, the only validation we do here to ensure that supported spread factors are used is the manual validation of the pools in AssetPairs?
There was a problem hiding this comment.
Yeah that is correct, we could for sure improve upon this but don't want to spend too much time on a feature that will be no longer needed by v18
Closes: #5892
What is the purpose of the change
Adds logic to upgrade handler to create CL pools that will be listed in a signaling proposal for v17. The new CL pools are linked to their gamm counterpart and, if SFS was previously enabled on that pair, is enabled on the new CL pool as well.
Testing and Verifying
Upgrade handler tests added to ensure:
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)