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.

Sign up for GitHub

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

Jump to bottom

[CL:Test] Add sqrtPrice check to swaps_test #4201

Merged
merged 17 commits into from
Feb 23, 2023
Merged

[CL:Test] Add sqrtPrice check to swaps_test #4201

merged 17 commits into from
Feb 23, 2023

Conversation

stackman27
Copy link
Contributor

@stackman27 stackman27 commented Feb 1, 2023 •
edited
Loading

Closes: #3989

What is the purpose of the change

checks sqrtPrice for TestCalcAndSwapOutAmtGivenIn, TestCalcAndSwapInAmtGivenOut

Brief Changelog

n/a

Testing and Verifying

n/a

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@stackman27 stackman27 requested review from czarcas7ic, p0mvn and mattverse and removed request for czarcas7ic and p0mvn February 1, 2023 23:02
@stackman27 stackman27 added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Feb 1, 2023
@czarcas7ic
Copy link
Member

Are we able to tie in with this PR these exact values but also derived from the python script Roman created? Otherwise we are just taking the word of whatever what output as the result which could potentially be incorrect.

@mattverse
Copy link
Member

mattverse commented Feb 2, 2023 •
edited
Loading

Are we able to tie in with this PR these exact values but also derived from the python script Roman created? Otherwise we are just taking the word of whatever what output as the result which could potentially be incorrect.

Good call! Will review once @czarcas7ic 's points has been addressed

stackman27
stackman27 commented Feb 6, 2023
View reviewed changes
scripts/cl/one_for_zero.py
@@ -2,30 +2,36 @@
import sympy as sp
from common import *


Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: all the python changes are from my prettifier. lmk if there's a specific preference and i'll reset them

@stackman27 stackman27 force-pushed the sis/sqrt-test branch 2 times, most recently from 65090b5 to 72d3e2d Compare February 6, 2023 22:05
@stackman27
Copy link
Contributor Author

Added wolfram calculation. Ready for rereview @czarcas7ic @mattverse!

@p0mvn
Copy link
Member

p0mvn commented Feb 8, 2023

Please do not merge this before the swap issues we're working on with Adam are resolved to avoid test breakage for the large PRs with fixes

@stackman27
Copy link
Contributor Author

Please do not merge this before the swap issues we're working on with Adam are resolved to avoid test breakage for the large PRs with fixes

Please lmk when the other PRs gets merged so that i can rebase and get this one ready!

@p0mvn
Copy link
Member

p0mvn commented Feb 17, 2023 •
edited
Loading

This is ready to be reviewed / merged once conflicts are resolved.

I attempted rebasing but most tests ended up failing. I'm not going to push to avoid breaking something here

p0mvn
p0mvn approved these changes Feb 21, 2023
View reviewed changes
x/concentrated-liquidity/swaps_test.go Outdated Show resolved Hide resolved
x/concentrated-liquidity/swaps_test.go Outdated Show resolved Hide resolved
@stackman27
Copy link
Contributor Author

@p0mvn resolved your comments. One extra thing to point out is the test comments are very misleading, we should definitely think about either removing them or adding new ones

@github-actions github-actions bot added C:app-wiring Changes to the app folder C:CLI C:docs Improvements or additions to documentation C:x/poolmanager C:x/tokenfactory C:x/twap Changes to the twap module labels Feb 23, 2023
@p0mvn
Copy link
Member

p0mvn commented Feb 23, 2023

@p0mvn resolved your comments. One extra thing to point out is the test comments are very misleading, we should definitely think about either removing them or adding new ones

Agreed, that's why for fee tests I've been moving towards estimating things via Python.

As we keep reviewing and refactoring swaps, I think we might want to do something similar for all other tests

@github-actions github-actions bot removed C:x/twap Changes to the twap module C:CLI C:x/poolmanager C:app-wiring Changes to the app folder C:x/tokenfactory C:docs Improvements or additions to documentation labels Feb 23, 2023
p0mvn
p0mvn approved these changes Feb 23, 2023
View reviewed changes
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you @stackman27 and sorry for breaking your branch earlier

@p0mvn p0mvn merged commit 52965de into main Feb 23, 2023
@p0mvn p0mvn deleted the sis/sqrt-test branch February 23, 2023 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@p0mvn p0mvn p0mvn approved these changes

@czarcas7ic czarcas7ic Awaiting requested review from czarcas7ic czarcas7ic is a code owner

@mattverse mattverse Awaiting requested review from mattverse

Assignees
No one assigned
Labels
C:x/concentrated-liquidity V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
Osmosis Chain Development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stackman27 @czarcas7ic @mattverse @p0mvn