feat(osmoutils): custom accumulator position updates #3903
Conversation
p0mvn
added
the
V:state/compatible/no_backport
| // | ||
| // An alternative approach is to simply generate an additional position every time an | ||
| // address adds to its position. We do not pursue this path because we want to ensure | ||
| // that withdrawal and claiming functions remain constant time and do not scale with the | ||
| // number of times a user has added to their position. |
There was a problem hiding this comment.
What do you think of removing this part?
| // | |
| // An alternative approach is to simply generate an additional position every time an | |
| // address adds to its position. We do not pursue this path because we want to ensure | |
| // that withdrawal and claiming functions remain constant time and do not scale with the | |
| // number of times a user has added to their position. |
There was a problem hiding this comment.
I think this is useful context
| @@ -35,8 +39,24 @@ func getPosition(accum AccumulatorObject, name string) (Record, error) { | |||
| func getTotalRewards(accum AccumulatorObject, position Record) sdk.DecCoins { | |||
| totalRewards := position.UnclaimedRewards | |||
|
|
|||
| // TODO: add a check that accum.value is greater than position.InitAccumValue | |||
There was a problem hiding this comment.
I reckon this is small enough to be included in this PR, was there a separate reason or further investigation you wanted to do before adding this check?
There was a problem hiding this comment.
This is non-blocking for fees. I don't want to get too wrapped up in the details to block further fee work.
We have a bunch of TODOs in this package since we still are iterating and prototyping the final design. I think it would make sense to come back and address all the details once we finalize the accum package and finish its integration into fees and incentives.
Let me know if that doesn't convince you to keep this as todo for now
| oldPositionAccumulatorValue: initialCoinsDenomOne.Add(sdk.NewDecCoin(initialCoinDenomOne.Denom, sdk.OneInt())), | ||
| expectError: accum.NegativeAccDifferenceError{sdk.NewDecCoins(sdk.NewDecCoin(initialCoinDenomOne.Denom, sdk.OneInt()))}, | ||
| }, | ||
| "old accumulator coins are a superset of new - error": { |
There was a problem hiding this comment.
| "old accumulator coins are a superset of new - error": { | |
| "old accumulator coins are not a superset of new - error": { |
There was a problem hiding this comment.
but they are in this test case. old coins contain coins from customAccumulatorValue plus other coins
| // All custom accumulator values must be non-negative. | ||
| // The position is initialized with empty unclaimed rewards | ||
| // If there is an existing position for the given address, it is overwritten. | ||
| func (accum AccumulatorObject) NewPositionCustomAcc(name string, numShareUnits sdk.Dec, customAccumulatorValue sdk.DecCoins, options *Options) error { |
There was a problem hiding this comment.
Nit: consider changing method name to NewPositionCustomizedAcc it was after reading through all other codes that I understood what this method meant. (Just a personal opinion tho so please feel free to resolve this issue if you think the current method name is more intuitive)
| func (accum AccumulatorObject) NewPositionCustomAcc(name string, numShareUnits sdk.Dec, customAccumulatorValue sdk.DecCoins, options *Options) error { | |
| func (accum AccumulatorObject) NewPositionCustomizedAcc(name string, numShareUnits sdk.Dec, customAccumulatorValue sdk.DecCoins, options *Options) error { |
There was a problem hiding this comment.
I've considered this but tbh I prefer the original NewPositionCustomAcc better because it is shorter. To me, there is no semantical difference between Custom and Customized.
If you don't mind, I will keep the old name for shortness but lmk if any concerns
osmoutils/accum/accum.go
Outdated
| func (accum AccumulatorObject) UpdatePositionCustomAcc(name string, numShares sdk.Dec, customAccumulatorValue sdk.DecCoins) error { | ||
| return accum.updatePosition(name, numShares, customAccumulatorValue) | ||
| } | ||
|
|
||
| func (accum AccumulatorObject) updatePosition(name string, numShares sdk.Dec, positionAccumulatorUpdate sdk.DecCoins) error { |
There was a problem hiding this comment.
why not directly expose updatePosition if this is the case?
There was a problem hiding this comment.
Good point. Refactored APIs here to reduce boilerplate: 54042ac
Co-authored-by: Matt, Park <[email protected]>
Co-authored-by: Matt, Park <[email protected]>
|
Addressed all comments, thanks @mattverse |
|
Discussed offline - merging to unblock fees work |
Closes: #XXX
What is the purpose of the change
This PR implements the ability to provide custom position accumulators for
NewPositionCustomAcc,AddToPositionCustomAcc, andRemoveFromPositionCustomAccaccumulator methods.It also implements a convenience method called
UpdateAccumulatorand its custom versionUpdatePositionCustomAcc. IfnumSharesis positive, it acts asAddToPosition, if negative, it acts asRemoveFromPosition. If zero, it fails.Testing and Verifying
This change added tests.
The tests for "custom accumulator" APIs only focus on test cases around the custom accumulator updates since their default versions go over all internal edge cases in-depth.
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? no