Feature/osmosis cl #1527
Feature/osmosis cl #1527
Conversation
* update osmosis-types to v22; add concentrated-liquidity and poolmanager * update osmosis-types usages for v22 types * ensure pool and math dirs are included; update codecs for inclusion
joe-bowman
requested review from
faddat,
minhngoc274 and
tropicaldog
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis comprehensive update introduces a variety of new functionalities and improvements across the Osmosis protocol, particularly enhancing the concentrated liquidity and pool management systems. Key additions include new message types for liquidity positions, pool creation, and incentives, alongside updates to error handling and parameter management. This aligns with the protocol's ongoing efforts to refine its DeFi capabilities and user experience. Changes
Assessment against linked issues
Poem
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 (
|
| ProtocolDataTypeCrescentParams ProtocolDataType = 13 | ||
| ProtocolDataTypeCrescentReserveAddressBalance ProtocolDataType = 14 | ||
| ProtocolDataTypeCrescentPoolCoinSupply ProtocolDataType = 15 | ||
| ProtocolDataTypeCrescentParams ProtocolDataType = 13 // Deprecated: Do not use. |
| @@ -35,17 +35,18 @@ const ( | |||
| ProtocolDataTypeOsmosisParams ProtocolDataType = 2 | |||
| ProtocolDataTypeLiquidToken ProtocolDataType = 3 | |||
| ProtocolDataTypeOsmosisPool ProtocolDataType = 4 | |||
| ProtocolDataTypeCrescentPool ProtocolDataType = 5 | |||
| ProtocolDataTypeSifchainPool ProtocolDataType = 6 | |||
| ProtocolDataTypeCrescentPool ProtocolDataType = 5 // Deprecated: Do not use. | |||
|
review: I skipped the imported code, and reviewed all of the code added. I wondered if we could remove code referencing sif and cresent. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1527 +/- ##
==========================================
- Coverage 62.97% 62.45% -0.53%
==========================================
Files 192 194 +2
Lines 13495 13609 +114
==========================================
+ Hits 8499 8500 +1
- Misses 4181 4294 +113
Partials 815 815
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Talked to @joe-bowman -- the deprecated fields are still there so we don't mess up numbering. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 42
Out of diff range and nitpick comments (6)
third-party-chains/osmosis-types/gamm/codec.go (1)
46-53: Clarify the usage ofModuleCdcand ensure it is only used where appropriate, as per the comments.Consider adding more detailed documentation on when it is appropriate to use
ModuleCdcand when it should be avoided.proto/osmosis/accum/v1beta1/accum.proto (1)
34-50: The description in the comments foraccum_value_per_shareis detailed and informative. However, ensure that the complexity of the explanation does not hinder understanding for new developers.Consider simplifying the comment or adding a reference to further documentation for clarity.
third-party-chains/osmosis-types/concentrated-liquidity/constants.go (1)
9-72: The constants defined in this file, such asMinInitializedTick,MaxSpotPrice, andAuthorizedTickSpacing, are crucial for the operation of concentrated liquidity pools. Ensure that these constants are consistent with the rest of the protocol and are documented clearly to avoid confusion.Consider adding more detailed comments to explain the purpose and usage of each constant, especially those that are critical to the protocol's operation.
third-party-chains/osmosis-types/concentrated-liquidity/events.go (1)
3-57: The event type constants defined in this file, such asTypeEvtCreatePositionandTypeEvtCollectIncentives, are used to track and manage events within the concentrated liquidity functionality. Ensure that these constants are used consistently across the protocol and are documented clearly.Consider adding more detailed comments to explain the purpose and usage of each event type, especially those that are critical to the protocol's operation.
third-party-chains/osmosis-types/concentrated-liquidity/math/precompute.go (1)
39-39: TODO comment detected. Ensure that this is tracked or resolved if necessary.third-party-chains/osmosis-types/concentrated-liquidity/model/pool.go (1)
123-124: Clarify the TODO comment regarding backward compatibility.The TODO comment in the
SpotPricemethod mentions maintaining backward compatibility but lacks clarity on the specific changes needed or the timeline. It would be beneficial to expand this comment to provide more context or link to relevant issues/documentation.
|
|
||
| func (e SqrtRootCalculationError) Error() string { | ||
| return fmt.Sprintf("issue calculating square root of price limit %s", e.SqrtPriceLimit) | ||
| } | ||
|
|
||
| type TickToSqrtPriceConversionError struct { | ||
| NextTick int64 | ||
| } | ||
|
|
||
| func (e TickToSqrtPriceConversionError) Error() string { | ||
| return fmt.Sprintf("could not convert next tick to nextSqrtPrice (%v)", e.NextTick) | ||
| } | ||
|
|
||
| type SwapNoProgressError struct { | ||
| PoolId uint64 | ||
| UserProvidedCoin sdk.Coin | ||
| } | ||
|
|
||
| func (e SwapNoProgressError) Error() string { | ||
| return fmt.Sprintf("ran out of iterations during swap. Possibly entered an infinite loop. Pool id (%d), user provided coin (%s)", e.PoolId, e.UserProvidedCoin) | ||
| } | ||
|
|
||
| type SwapNoProgressWithConsumptionError struct { | ||
| ComputedSqrtPrice osmomath.BigDec | ||
| AmountIn osmomath.Dec | ||
| AmountOut osmomath.Dec | ||
| } | ||
|
|
||
| func (e SwapNoProgressWithConsumptionError) Error() string { | ||
| return fmt.Sprintf("did not advance sqrt price after swap step %s, with amounts in (%s), out (%s)", e.ComputedSqrtPrice, e.AmountIn, e.AmountOut) | ||
| } | ||
|
|
||
| type SqrtPriceToTickError struct { | ||
| OutOfBounds bool | ||
| } | ||
|
|
||
| func (e SqrtPriceToTickError) Error() string { | ||
| return fmt.Sprintf("sqrt price to tick could not find a satisfying tick index. Hit bounds: %v", e.OutOfBounds) | ||
| } | ||
|
|
||
| type OverChargeSwapOutGivenInError struct { | ||
| AmountSpecifiedRemaining osmomath.Dec | ||
| } | ||
|
|
||
| func (e OverChargeSwapOutGivenInError) Error() string { | ||
| return fmt.Sprintf("over charge problem swap out given in by (%s)", e.AmountSpecifiedRemaining) | ||
| } | ||
|
|
||
| type ComputedSqrtPriceInequalityError struct { | ||
| IsZeroForOne bool | ||
| NextInitializedTickSqrtPrice osmomath.BigDec | ||
| ComputedSqrtPrice osmomath.BigDec | ||
| } | ||
|
|
||
| func (e ComputedSqrtPriceInequalityError) Error() string { | ||
| return fmt.Sprintf("edge case has occurred when swapping at tick boundaries, with izZeroForOne (%t), NextInitializedTickSqrtPrice (%s), computedSqrtPrice (%s). Please try again with a different swap amount", e.IsZeroForOne, e.NextInitializedTickSqrtPrice, e.ComputedSqrtPrice) | ||
| } | ||
|
|
||
| type NumberOfTicksExceedsMaxError struct { | ||
| NumberOfTicks uint64 | ||
| MaxNumberOfTicks uint64 | ||
| } | ||
|
|
||
| func (e NumberOfTicksExceedsMaxError) Error() string { | ||
| return fmt.Sprintf("number of ticks exceeds max ticks. Provided number of ticks (%d), max ticks (%d)", e.NumberOfTicks, e.MaxNumberOfTicks) | ||
| } | ||
|
|
||
| type DuplicatePositionIdsError struct { | ||
| PositionIds []uint64 | ||
| } | ||
|
|
||
| func (e DuplicatePositionIdsError) Error() string { | ||
| return fmt.Sprintf("duplicate position ids found (%v)", e.PositionIds) | ||
| } | ||
|
|
||
| type LastPositionTransferError struct { | ||
| PositionId uint64 | ||
| PoolId uint64 | ||
| } | ||
|
|
||
| func (e LastPositionTransferError) Error() string { | ||
| return fmt.Sprintf("cannot transfer position %d because it is the last position in pool %d", e.PositionId, e.PoolId) | ||
| } | ||
|
|
||
| type ContractHookOutOfGasError struct { | ||
| GasLimit uint64 | ||
| } | ||
|
|
||
| func (e ContractHookOutOfGasError) Error() string { | ||
| return fmt.Sprintf("A single contract call cannot exceed %d gas in a CL hook call.", e.GasLimit) | ||
| } | ||
|
|
||
| type InvalidActionPrefixError struct { | ||
| ActionPrefix string | ||
| ValidActions []string | ||
| } | ||
|
|
||
| func (e InvalidActionPrefixError) Error() string { | ||
| return fmt.Sprintf("invalid action prefix (%s). Valid actions: %s", e.ActionPrefix, e.ValidActions) | ||
| } |
There was a problem hiding this comment.
Ensure minimal changes to vendored files to maintain alignment with the original source.
This file is part of a vendored directory, and as per previous discussions, changes to these files should be minimal to keep them as close to the original as possible. If you need assistance in aligning the changes with the original source, please let me know.
| // NewConcentratedLiquidityPool creates a new ConcentratedLiquidity pool with the specified parameters. | ||
| // The two provided denoms are ordered so that denom0 is lexicographically smaller than denom1. | ||
| func NewConcentratedLiquidityPool(poolId uint64, denom0, denom1 string, tickSpacing uint64, spreadFactor osmomath.Dec) (Pool, error) { | ||
| // Ensure that the two denoms are different | ||
| if denom0 == denom1 { | ||
| return Pool{}, types.MatchingDenomError{Denom: denom0} | ||
| } | ||
|
|
||
| // Spread factor must be [0,1) | ||
| if spreadFactor.IsNegative() || spreadFactor.GTE(one) { | ||
| return Pool{}, types.InvalidSpreadFactorError{ActualSpreadFactor: spreadFactor} | ||
| } | ||
|
|
||
| // Create a new pool struct with the specified parameters | ||
| pool := Pool{ | ||
| Address: poolmanagertypes.NewPoolAddress(poolId).String(), | ||
| IncentivesAddress: osmoutils.NewModuleAddressWithPrefix(types.ModuleName, incentivesAddressPrefix, sdk.Uint64ToBigEndian(poolId)).String(), | ||
| SpreadRewardsAddress: osmoutils.NewModuleAddressWithPrefix(types.ModuleName, spreadRewardsAddressPrefix, sdk.Uint64ToBigEndian(poolId)).String(), | ||
| Id: poolId, | ||
| CurrentSqrtPrice: osmomath.ZeroBigDec(), | ||
| CurrentTick: 0, | ||
| CurrentTickLiquidity: osmomath.ZeroDec(), | ||
| Token0: denom0, | ||
| Token1: denom1, | ||
| TickSpacing: tickSpacing, | ||
| ExponentAtPriceOne: types.ExponentAtPriceOne, | ||
| SpreadFactor: spreadFactor, | ||
| } | ||
| return pool, nil |
There was a problem hiding this comment.
Ensure the new NewConcentratedLiquidityPool function adheres to minimal modification principles for vendored files.
This function introduces significant new logic for creating a concentrated liquidity pool. Given that this file is from a vendored directory, which should ideally remain close to the original to ease updates and maintain compatibility, please confirm if these changes are essential and if they have been or will be contributed back to the source repository.
| // GetAddress returns the address of the concentrated liquidity pool | ||
| func (p Pool) GetAddress() sdk.AccAddress { | ||
| addr, err := addressutils.AccAddressFromBech32(p.Address, "") | ||
| if err != nil { | ||
| panic(fmt.Sprintf("could not bech32 decode address of pool with id: %d", p.GetId())) | ||
| } | ||
| return addr | ||
| } |
There was a problem hiding this comment.
Review the use of panic for error handling in GetAddress.
Using panic for error handling in production code, especially in a blockchain context where stability is crucial, is risky. It can lead to abrupt termination of the application. Consider returning the error to the caller and handling it gracefully.
| // GetIncentivesAddress returns the address storing incentives of the concentrated liquidity pool. | ||
| func (p Pool) GetIncentivesAddress() sdk.AccAddress { | ||
| addr, err := addressutils.AccAddressFromBech32(p.IncentivesAddress, "") | ||
| if err != nil { | ||
| panic(fmt.Sprintf("could not bech32 decode incentive address of pool with id: %d", p.GetId())) | ||
| } | ||
| return addr |
There was a problem hiding this comment.
Review the use of panic for error handling in GetIncentivesAddress.
Similar to the previous comment, the use of panic here can lead to instability. It's generally better practice in Go to handle errors gracefully by returning them to the caller, unless a panic is absolutely necessary.
| func (p Pool) GetSpreadRewardsAddress() sdk.AccAddress { | ||
| addr, err := addressutils.AccAddressFromBech32(p.SpreadRewardsAddress, "") | ||
| if err != nil { | ||
| panic(fmt.Sprintf("could not bech32 decode spread factor address of pool with id: %d", p.GetId())) |
There was a problem hiding this comment.
Review the use of panic for error handling in GetSpreadRewardsAddress.
The use of panic for handling decoding errors is not recommended. Consider modifying the method to return an error and handle it appropriately in the calling code.
| * `accum/pos/{accumName}||{positionName}` | ||
|
|
||
| We really should be prefix separating this state into its own sub-area (or potentially even a different store entirely -- I think its worth doing this prelaunch) | ||
|
|
There was a problem hiding this comment.
Correct the spelling mistake in "AccumName".
- accumName and positionName's are confined to one of the following two:
+ AccumName and PositionName's are confined to one of the following two:Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
|
|
||
| We really should be prefix separating this state into its own sub-area (or potentially even a different store entirely -- I think its worth doing this prelaunch) | ||
|
|
||
| accumName and positionName's are confined to one of the following two: |
There was a problem hiding this comment.
Correct the spelling mistake in "AccumName".
- - Spread rewards - accumName = `0x0B/` || `str encode pool ID`
+ - Spread rewards - AccumName = `0x0B/` || `string encode pool ID`Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| accumName and positionName's are confined to one of the following two: | |
| accumName and positionName's are confined to one of the following two: |
| accumName and positionName's are confined to one of the following two: | ||
| - Spread rewards | ||
| - accumName = `0x0B/` || `str encode pool ID` | ||
| - positionName = `0x0A/` || `str encode position ID` |
There was a problem hiding this comment.
Correct the spelling mistake in "PositionName".
- - positionName = `0x0A/` || `str encode position ID`
+ - PositionName = `0x0A/` || `string encode position ID`Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - positionName = `0x0A/` || `str encode position ID` | |
| - PositionName = `0x0A/` || `string encode position ID` |
| - accumName = `0x0B/` || `str encode pool ID` | ||
| - positionName = `0x0A/` || `str encode position ID` | ||
| - incentive rewards | ||
| - accumName = `0x0C/` || `str encode pool ID` || `/` || `str encode uptime index ID` |
There was a problem hiding this comment.
Correct the spelling mistake in "AccumName".
- - accumName = `0x0C/` || `str encode pool ID` || `/` || `str encode uptime index ID`
+ - AccumName = `0x0C/` || `string encode pool ID` || `/` || `string encode uptime index ID`Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - accumName = `0x0C/` || `str encode pool ID` || `/` || `str encode uptime index ID` | |
| - AccumName = `0x0C/` || `string encode pool ID` || `/` || `string encode uptime index ID` |
| - positionName = `0x0A/` || `str encode position ID` | ||
| - incentive rewards | ||
| - accumName = `0x0C/` || `str encode pool ID` || `/` || `str encode uptime index ID` | ||
| - positionName = `0x08` || `var-length, base10 string encoding of position ID` |
There was a problem hiding this comment.
Correct the spelling mistake in "PositionName".
- - positionName = `0x08` || `var-length, base10 string encoding of position ID`
+ - PositionName = `0x08` || `var-length, base10 string encoding of position ID`Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - positionName = `0x08` || `var-length, base10 string encoding of position ID` | |
| - PositionName = `0x08` || `var-length, base10 string encoding of position ID` |
1. Summary
Fixes #1151
Update's osmosis-types dependency (can import properly when we are on SDK 47!)
Add CL Pool Submodule for participation rewards