Skip to content
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

refactor(twap): add sanity check in updateRecord to compare record time and height against ctx #2686

Closed
Tracked by #4622
p0mvn opened this issue Sep 9, 2022 · 6 comments · Fixed by #2741
Closed
Tracked by #4622
Assignees

Comments

@p0mvn
Copy link
Member

p0mvn commented Sep 9, 2022

Background

This was observed during testing of updateRecords. It is possible to set up a testing environment in such a way as to be able to insert a record between the existing records.

For example, assume a two-asset pool and 2 records at the times t and t + 2. If I set up ctx.BlockTime to be t + 1, I can insert a new record in-between without error.

While this should not be possible to reproduce in real environment, having a sanity check in updateRecord to ensure the following invariants would be useful:

  • ctx.BlockTime > record.Time
  • ctx.BlockHeight > record.Height

updateRecord and updateRecords should be returning an error in these cases.

Suggested Design

Return error if any of the above invariants are not satisfied.

Acceptance Criteria

  • invariants are added to updateRecord
  • new code branches of updateRecord are covered by tests
  • existing updateRecords tests are refactored / adjusted to cover the new behavior
@p0mvn p0mvn added T:tests C:x/twap Changes to the twap module labels Sep 9, 2022
@p0mvn p0mvn added this to the V12 Blockers milestone Sep 9, 2022
@p0mvn p0mvn moved this to Needs Review 🔍 in Osmosis Chain Development Sep 9, 2022
@p0mvn p0mvn changed the title refactor(twap): add sanity check in update record to compare its time against ctx.BlockTime refactor(twap): add sanity check in update record to compare record time and height against ctx Sep 9, 2022
@p0mvn p0mvn changed the title refactor(twap): add sanity check in update record to compare record time and height against ctx refactor(twap): add sanity check in update record to compare record time and height against ctx Sep 9, 2022
@p0mvn p0mvn changed the title refactor(twap): add sanity check in update record to compare record time and height against ctx refactor(twap): add sanity check in updateRecord to compare record time and height against ctx Sep 9, 2022
@hieuvubk
Copy link
Contributor

"Uni2 Pool, swap on pool creation block": {

@p0mvn We have an exception here. With sanity check added, swap on pool creation block should return err. What do u think about that?

@hieuvubk hieuvubk self-assigned this Sep 14, 2022
@hieuvubk
Copy link
Contributor

hieuvubk commented Sep 21, 2022

"Uni2 Pool, swap on pool creation block": {

@p0mvn We have an exception here. With sanity check added, swap on pool creation block should return err. What do u think about that?

@hieuvubk
Copy link
Contributor

"Uni2 Pool, swap on pool creation block": {

@p0mvn We have an exception here. With sanity check added, swap on pool creation block should return err. What do u think about that?

I think we should discuss with @mattverse owning this commit

@mattverse
Copy link
Member

@hieuvubk What causes the error? At a light glance, I don't see anything calling updateRecord with in the afterPoolCreation test

@hieuvubk
Copy link
Contributor

hieuvubk commented Sep 23, 2022

Sr to bother , I checked actually this comes from EndBlock( just logged the err) when updating the record created in the same block in afterCreatePool.
So I updated the condition again by checking the case when creating the pool. And should we use twapAccumulator variables to check the record is unedited?

@hieuvubk
Copy link
Contributor

hieuvubk commented Apr 6, 2023

@p0mvn @mattverse Reopend this pr cause its still available for issue. Could u review it? Thanksss

@github-project-automation github-project-automation bot moved this from Needs Triage 🔍 to Done ✅ in Osmosis Chain Development May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants