fix: migrate subset of spread reward accums #7876
Conversation
czarcas7ic
added
V:state/compatible/no_backport
github-actions
bot
added
C:app-wiring
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Actionable comments outside the diff hunks (10)
CHANGELOG.md (10)
Line range hint
325-325: Ensure headings increment by one level at a time for proper document structure.-## v6.4.0 +### v6.4.0
Line range hint
986-986: Ensure headings increment by one level at a time for proper document structure.-## v6.3.1 +### v6.3.1
Line range hint
998-998: Ensure headings increment by one level at a time for proper document structure.-## v6.3.0 +### v6.3.0
Line range hint
1007-1007: Ensure headings increment by one level at a time for proper document structure.-## v6.2.0 +### v6.2.0
Line range hint
1012-1012: Ensure headings increment by one level at a time for proper document structure.-## v6.1.0 +### v6.1.0
Line range hint
1064-1064: Ensure headings increment by one level at a time for proper document structure.-## v6.0.0 +### v6.0.0
Line range hint
325-325: Remove trailing punctuation from heading for consistency.-## [v6.0.0] +## [v6.0.0]
Line range hint
588-588: Convert bare URLs to markdown links for better readability and navigation.-https://github.com/osmosis-labs/osmosis/releases/tag/v6.0.0 +[v6.0.0](https://github.com/osmosis-labs/osmosis/releases/tag/v6.0.0)
Line range hint
646-646: Convert bare URLs to markdown links for better readability and navigation.-https://github.com/osmosis-labs/osmosis/releases/tag/v6.0.0 +[v6.0.0](https://github.com/osmosis-labs/osmosis/releases/tag/v6.0.0)
Line range hint
1227-1227: Ensure all links are properly formatted and not empty.-[] +[Proper link text](link-url)
| // Truncated decimal, which is dust that is added back to the global accumulator | ||
| truncatedDec = scaledDec.Sub(scaledAmount.ToLegacyDec()) |
There was a problem hiding this comment.
If removing this doesn't lead to significant amounts of fees becoming unclaimable I'd be in support of not doing it
| // Update global spread reward growth with the UNSCALED total | ||
| // We could scale this up, but we would need to scale it right back down at the end of the swap. | ||
| // The globalSpreadRewardGrowth is used to determine how much the user sends to the spread reward address, | ||
| // and is not persisted in the pool's tick accumulators. | ||
| ss.globalSpreadRewardGrowth = ss.globalSpreadRewardGrowth.Add(spreadRewardChargeTotal) |
There was a problem hiding this comment.
Yeah this looks fine since globalSpreadRewardGrowth is initialized to zero and only used to settle balances post-swap. It does break the boundary of "only operate in scaled space for swaps" but if it's an important performance improvement I think it's okay to keep
I was going to suggest adding a comment on the field in swap strategy proto but I see you've done that already!
Co-authored-by: Alpo <62043214+AlpinYukseloglu@users.noreply.github.com>
Co-authored-by: Alpo <62043214+AlpinYukseloglu@users.noreply.github.com>
Co-authored-by: Alpo <62043214+AlpinYukseloglu@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
x/concentrated-liquidity/swaps_test.go (1)
737-770: Consider adding comments to explain the purpose of the test case.Adding comments to the test cases can help other developers understand the purpose and expected behavior of each test, improving maintainability.
| // TODO: Dont hardcode one here | ||
| swapState.UpdateSpreadRewardGrowthGlobal(tc.spreadRewardChargeTotal, sdk.OneDec()) |
There was a problem hiding this comment.
Remove unnecessary TODO comment or address it if it's still relevant.
- // TODO: Dont hardcode one here
- // TODO: Dont hardcode one hereThis TODO comment suggests a potential issue with hardcoding values. If this is still a concern, it should be addressed by parameterizing the value or explaining why it is hardcoded. If it's no longer relevant, the comment should be removed to avoid confusion.
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.
| // TODO: Dont hardcode one here | |
| swapState.UpdateSpreadRewardGrowthGlobal(tc.spreadRewardChargeTotal, sdk.OneDec()) | |
| swapState.UpdateSpreadRewardGrowthGlobal(tc.spreadRewardChargeTotal, sdk.OneDec()) |
| sortedPermAddrs := make([]string, 0, len(k.permAddrs)) | ||
| for moduleName := range k.permAddrs { | ||
| sortedPermAddrs = append(sortedPermAddrs, moduleName) | ||
| } | ||
| sort.Strings(sortedPermAddrs) |
There was a problem hiding this comment.
Consider caching sorted permission addresses to improve performance.
Sorting permAddrs every time forceTransfer is called could lead to performance issues, especially if the number of permissions is large. Consider caching the sorted list or ensuring that it remains sorted as permissions are added or removed.
Closes: #XXX
What is the purpose of the change
This feature is essentially identical to the work that was done on the incentive accumulators. There exists conditions in which dividing the spread factor by the liquidity amount results in truncation to zero, resulting in the proper amount of spread factor to be deposited into the spread factor module account, but not be distributed to LPs.
This will be fixed in two stages just like incentives:
Testing and Verifying
TBD
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests
Chores