[NonPoolSuperfluid] Use full routes instead of only one pool for pricing #8276
Conversation
…ative is properly tested
…ns aren't returned after delegation ends
…ependent-risk-factor
nicolaslara
requested review from
p0mvn,
AlpinYukseloglu,
mattverse and
czarcas7ic
as code owners
WalkthroughThe recent changes expand support for non-pool assets in superfluid staking, enhancing functionality by integrating multi-pool arithmetic TWAP calculations. Key updates include modifying the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant SuperfluidKeeper
participant TwapKeeper
participant PoolManager
User->>SuperfluidKeeper: Request to add Superfluid Asset
SuperfluidKeeper->>TwapKeeper: Validate asset using GetMultiPoolArithmeticTwapToNow
TwapKeeper->>PoolManager: Fetch Swap Routes
PoolManager-->>TwapKeeper: Return Swap Routes
TwapKeeper-->>SuperfluidKeeper: Return TWAP Price
SuperfluidKeeper-->>User: Asset Added Successfully
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (10)
proto/osmosis/superfluid/superfluid.proto (1)
33-35: Ensure theprice_routefield is properly documented.Consider adding more detailed comments for the
price_routefield to explain its usage and the expected format ofSwapAmountInRoute.CHANGELOG.md (9)
Line range hint
635-635: Avoid using bare URLs; provide descriptive link text.- * [sdk-#100](https://github.com/osmosis-labs/cosmos-sdk/pull/100) Upgrade iavl with fast storage + * [Upgrade iavl with fast storage](https://github.com/osmosis-labs/cosmos-sdk/pull/100)
Line range hint
693-693: Avoid using bare URLs; provide descriptive link text.- * [iavl-5](https://github.com/osmosis-labs/iavl/pull/5) Fast storage optimization for queries and iterations + * [Fast storage optimization for queries and iterations](https://github.com/osmosis-labs/iavl/pull/5)
Line range hint
1033-1033: Correct the heading level for better document structure.- #### Golang API breaks + ### Golang API breaks
Line range hint
1045-1045: Correct the heading level for better document structure.- #### Bug Fixes + ### Bug Fixes
Line range hint
1054-1054: Correct the heading level for better document structure.- #### Features + ### Features
Line range hint
1059-1059: Correct the heading level for better document structure.- #### Minor improvements & Bug Fixes + ### Minor improvements & Bug Fixes
Line range hint
1111-1111: Correct the heading level for better document structure.- #### SDK fork updates + ### SDK fork updates
Line range hint
372-372: Remove trailing punctuation in heading for consistency.- ## v6.4.0. + ## v6.4.0
Line range hint
1274-1274: Ensure all links have descriptive text instead of being empty.Please provide descriptive text for the link at line 1274 to improve accessibility and context.
nicolaslara
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
CHANGELOG.md (4)
Line range hint
1058-1058: Adjust heading levels to increment by one level at a time for better document structure.- ### State Breaking + ## State Breaking - ### Features + ## Features - ### Bug Fixes + ## Bug Fixes - ### Misc Improvements + ## Misc Improvements - ### SDK fork updates + ## SDK fork updatesAlso applies to: 1070-1070, 1079-1079, 1084-1084, 1136-1136
Line range hint
397-397: Remove trailing punctuation from the heading for consistency.- ## v6.4.0. + ## v6.4.0
Line range hint
660-660: Replace bare URLs with descriptive links to improve readability.- [sdk-#100](https://github.com/osmosis-labs/cosmos-sdk/pull/100) + [sdk-#100](https://github.com/osmosis-labs/cosmos-sdk/pull/100 "Upgrade iavl with fast storage") - [iavl-5](https://github.com/osmosis-labs/iavl/pull/5) + [iavl-5](https://github.com/osmosis-labs/iavl/pull/5 "Fast storage optimization for queries and iterations")Also applies to: 718-718
Line range hint
1299-1299: Ensure all links are functional and not empty.- [v6.4.0](https://github.com/osmosis-labs/osmosis/releases/tag/v6.4.0) + [v6.4.0](https://github.com/osmosis-labs/osmosis/releases/tag/v6.4.0 "View the v6.4.0 release details")
| // Validators get stake and uosmo. I think this is an issue with test setup | ||
| s.Require().Equal(2, len(validatorRewards.Rewards.Rewards)) |
There was a problem hiding this comment.
Would like to get to the bottom of this prior to merge
There was a problem hiding this comment.
The reason for this is that what gets distributed is every collected fee. That comes from checking the fee collector's balances here: https://github.com/cosmos/cosmos-sdk/blob/main/x/distribution/keeper/allocation.go#L23
Somewhere along the line, someone is sending stake to the fee collector (haven't figured out where, but I think that's ok). The behaviour of distributing that fee is correct though
There was a problem hiding this comment.
Sounds good, lets add a TODO to either prevent this other distribution from happening, or in the comments add explicit detail of where this other send is coming from and how to verify.
There was a problem hiding this comment.
Double emission in test fixed here 63e48b5
| // Validators get stake and uosmo. I think this is an issue with test setup | ||
| s.Require().Equal(2, len(validatorRewards.Rewards.Rewards)) |
There was a problem hiding this comment.
Sounds good, lets add a TODO to either prevent this other distribution from happening, or in the comments add explicit detail of where this other send is coming from and how to verify.
x/twap/api.go
Outdated
| // Only pools with two assets are considered. | ||
| // For each pool n, its base asset will be the quote asset of pool n-1. The first pool's base asset is specified | ||
| // in baseAssetDenom and the last pool's quote asset is specified in quoteAssetDenom. | ||
| func (k Keeper) GetMultiPoolArithmeticTwapToNow( |
There was a problem hiding this comment.
I think this needs testing in the respective api_test.go file, despite it being used in the integration test.
Would personally like to see a test case that convinces someone that the resulting twap is transformed properly to the quote asset (so maybe two very different asset precisions)
There was a problem hiding this comment.
We need some warnings here for this not actually being the correct TWAP of the pair.
We want the average value of
This PR is returning
These are not the same.
So we need to be careful about the precise usages here, or consider bucketing into many smaller intervals for a big TWAP, to improve accuracy.
There was a problem hiding this comment.
TL;DR average(a*b) != average(a)*average(b)
We can add a comment saying this is likely an acceptable approximation error in the following two cases:
- if one asset is a stable pair we are fine assuming is basically constant for the time range
- If all assets involved are major assets, and we rely on a proto-rev cyclic arb assumption. Then the result should be within a (guess-timate) 1-3% error tolerance. (roughly ~.3% * num_pools in path to close arb)
There was a problem hiding this comment.
Explanation added here 3509723, thanks for breaking this down
There was a problem hiding this comment.
Also, tests and bug in the method corrected here be2a32b
There was a problem hiding this comment.
I think solving it with documentation makes sense here, as what we want is the "current price of an asset through a route, based on each historical twap" even if that isn't the theoretical twap of the route.
@ValarDragon I understand that they aren't the same for a twap of the route, but do you agree that the product of averages is enough to determine an accepted price of the route at the time queried? Just double checking my understanding here, but I think for errors to accumulate we would need each pool in the route to be mispriced in the same direction.
There was a problem hiding this comment.
I think its enough to determine an acceptable price in the circumstances were concerned about! The general case requires us to be careful / think about no possible tail explosions. (E.g. we don't weight pools by liquidity at time, so if you averaged across routes, or even in a route with one low liquidity pool, you could get non-sensical values)
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
x/twap/api.go (1)
150-195: Consider adding more detailed documentation about the limitations and potential inaccuracies of theUnsafeGetMultiPoolArithmeticTwapToNowmethod to ensure users are fully aware of the conditions under which it can be used safely.Tools
golangci-lint
166-166: undefined: Keeper
I am pretty sure the way we set I solved in this commit, but will need ACK be2a32b |
|
Thanks for the fix and tests @czarcas7ic ! |
Closes: #XXX
What is the purpose of the change
Allow native superfluid to use a full route instead of only one pool
Testing and Verifying
tests updated
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)