-
Notifications
You must be signed in to change notification settings - Fork 607
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove using an iterator from updating TWAP records #7266
Conversation
@@ -151,6 +151,23 @@ func (k Keeper) GetAllMostRecentRecordsForPool(ctx sdk.Context, poolId uint64) ( | |||
return types.GetAllMostRecentTwapsForPool(store, poolId) | |||
} | |||
|
|||
// GetAllMostRecentRecordsForPool returns all most recent twap records | |||
// (in state representation) for the provided pool id. | |||
func (k Keeper) GetAllMostRecentRecordsForPoolWithDenoms(ctx sdk.Context, poolId uint64, denoms []string) ([]types.TwapRecord, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have unit test for this method?
// (in state representation) for the provided pool id. | ||
func (k Keeper) GetAllMostRecentRecordsForPoolWithDenoms(ctx sdk.Context, poolId uint64, denoms []string) ([]types.TwapRecord, error) { | ||
store := ctx.KVStore(k.storeKey) | ||
// if length != 2, use iterator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice 🌮
I think we should add changelog for this, but is soft suggestion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think we should have the unit test as Matt suggested though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
func GetMostRecentTwapForPool(store sdk.KVStore, poolId uint64, denom1, denom2 string) (TwapRecord, error) { | ||
key := FormatMostRecentTWAPKey(poolId, denom1, denom2) | ||
bz := store.Get(key) | ||
return ParseTwapFromBz(bz) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: would it be better to have this within keeper since it touches store?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having this here makes sense given prev code structure (e.g GetAllMostRecentTwapsForPool also lives in types/keys.go)
Agree about the unit test |
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
Added unit tests, will be merging this PR by EOW |
Thank you for adding the test 🙏 |
* Remove using an iterator from updating TWAP records * Changelog * Add test case --------- Co-authored-by: mattverse <[email protected]> Co-authored-by: Matt, Park <[email protected]> (cherry picked from commit 9193ee4) # Conflicts: # CHANGELOG.md
* Remove using an iterator from updating TWAP records * Changelog * Add test case --------- Co-authored-by: mattverse <[email protected]> Co-authored-by: Matt, Park <[email protected]> (cherry picked from commit 9193ee4)
* Remove using an iterator from updating TWAP records * Changelog * Add test case --------- Co-authored-by: mattverse <[email protected]> Co-authored-by: Matt, Park <[email protected]> (cherry picked from commit 9193ee4) Co-authored-by: Dev Ojha <[email protected]>
…7403) * Remove using an iterator from updating TWAP records (#7266) * Remove using an iterator from updating TWAP records * Changelog * Add test case --------- Co-authored-by: mattverse <[email protected]> Co-authored-by: Matt, Park <[email protected]> (cherry picked from commit 9193ee4) # Conflicts: # CHANGELOG.md * Update CHANGELOG.md --------- Co-authored-by: Dev Ojha <[email protected]>
This shows up in benchmarks as being a slowdown on getting TWAP records. It will affect gas, but should only do so in EndBlock logic which should not be gas metered.
We see this iterator call being the cause of around .4% of State machine processing speed and even persists in the IAVL v2 benchmarks as a real expense.