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

refactor: dry swap #421

Merged
merged 30 commits into from
Dec 16, 2024
Merged

refactor: dry swap #421

merged 30 commits into from
Dec 16, 2024

Conversation

notJoon
Copy link
Member

@notJoon notJoon commented Dec 6, 2024 •
edited
Loading

Description

  • Refactor DrySwap function
  • Combine into one file with the Swap function
  • Add test

@notJoon notJoon added refactoring pool Pool contract related tasks labels Dec 10, 2024
@notJoon notJoon self-assigned this Dec 10, 2024
@notJoon notJoon marked this pull request as ready for review December 10, 2024 11:29
r3v4s
r3v4s reviewed Dec 10, 2024
View reviewed changes
pool/swap.gno Outdated Show resolved Hide resolved
r3v4s
r3v4s reviewed Dec 10, 2024
View reviewed changes
pool/swap.gno Outdated Show resolved Hide resolved
r3v4s
r3v4s reviewed Dec 10, 2024
View reviewed changes
pool/swap.gno Outdated Show resolved Hide resolved
r3v4s
r3v4s reviewed Dec 10, 2024
View reviewed changes
pool/swap.gno Outdated Show resolved Hide resolved
r3v4s
r3v4s reviewed Dec 10, 2024
View reviewed changes
pool/swap.gno Outdated Show resolved Hide resolved
r3v4s
r3v4s reviewed Dec 10, 2024
View reviewed changes
pool/swap.gno Outdated Show resolved Hide resolved
r3v4s
r3v4s reviewed Dec 10, 2024
View reviewed changes
pool/swap.gno Outdated Show resolved Hide resolved
r3v4s
r3v4s requested changes Dec 10, 2024
View reviewed changes
Copy link
Member

@r3v4s r3v4s left a comment

Choose a reason for hiding this comment

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

Can you add test cases that Dryswap() and Swap() will return same amount ?

dongwon8247
dongwon8247 reviewed Dec 10, 2024
View reviewed changes
pool/swap_test.gno Outdated Show resolved Hide resolved
onlyhyde
onlyhyde requested changes Dec 11, 2024
View reviewed changes
pool/swap.gno Outdated Show resolved Hide resolved
onlyhyde
onlyhyde requested changes Dec 11, 2024
View reviewed changes
pool/swap.gno Outdated Show resolved Hide resolved
notJoon and others added 5 commits December 11, 2024 17:50
@notJoon
fix
7631ecb
@onlyhyde
GSW-1838 refactor: Using the new constructor to protect raw data in s…
8b83678 
…qrtRatio calculations

- fix bug of test case in position_modify_test
- remove print log
@onlyhyde
GSW-1838 refactor: need to lock pool.slot0 to prevent re-entry
53bc59f
@onlyhyde
refactor: Using clone data to protect original data
7d3f7db 
- Changing variable names by applying refactoring rules
- Removing paths from error messages
@onlyhyde
Merge branch 'main' into dry-swap
e11c132
@notJoon notJoon requested a review from onlyhyde December 11, 2024 10:47
r3v4s
r3v4s reviewed Dec 15, 2024
View reviewed changes
pool/swap_test.gno Outdated Show resolved Hide resolved
r3v4s
r3v4s previously approved these changes Dec 15, 2024
View reviewed changes
Copy link
Member

@r3v4s r3v4s left a comment

Choose a reason for hiding this comment

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

test cases run successful with my env 👍
left some comments, but minor thing (does not affect logic)

@onlyhyde onlyhyde dismissed stale reviews from r3v4s and dongwon8247 via 71823f1 December 15, 2024 16:18
dongwon8247
dongwon8247 previously approved these changes Dec 15, 2024
View reviewed changes
r3v4s
r3v4s previously approved these changes Dec 16, 2024
View reviewed changes
Copy link
Member

@r3v4s r3v4s left a comment

Choose a reason for hiding this comment

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

👍

notJoon
notJoon commented Dec 16, 2024
View reviewed changes
Copy link
Member Author

@notJoon notJoon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. I left some minor comments; don't have to apply these now.

_deploy/p/gnoswap/pool/sqrt_price_math.gno Outdated Show resolved Hide resolved
pool/liquidity_math.gno Outdated Show resolved Hide resolved
pool/liquidity_math.gno Outdated Show resolved Hide resolved
pool/type.gno Show resolved Hide resolved
_deploy/p/gnoswap/pool/sqrt_price_math.gno Outdated Show resolved Hide resolved
_deploy/r/gnoswap/common/tick_math.gno Outdated Show resolved Hide resolved
pool/liquidity_math.gno Outdated Show resolved Hide resolved
pool/liquidity_math.gno Outdated Show resolved Hide resolved
_deploy/r/gnoswap/common/tick_math.gno Show resolved Hide resolved
@onlyhyde onlyhyde dismissed stale reviews from r3v4s and dongwon8247 via d2bb70c December 16, 2024 02:42
onlyhyde
onlyhyde previously approved these changes Dec 16, 2024
View reviewed changes
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
3.9% Duplication on New Code

See analysis details on SonarQube Cloud

@onlyhyde
Copy link
Member

@notJoon Updated the description of the comment.

r3v4s
r3v4s approved these changes Dec 16, 2024
View reviewed changes
Copy link
Member

@r3v4s r3v4s left a comment

Choose a reason for hiding this comment

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

👍

notJoon
notJoon commented Dec 16, 2024
View reviewed changes
Copy link
Member Author

@notJoon notJoon left a comment

Choose a reason for hiding this comment

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

👍

@onlyhyde onlyhyde merged commit a94f232 into main Dec 16, 2024
1 of 2 checks passed
@onlyhyde onlyhyde deleted the dry-swap branch December 16, 2024 03:51
onlyhyde added a commit that referenced this pull request Dec 18, 2024
@notJoon @onlyhyde @r3v4s
refactor: dry swap (#421)
168c7df 
* remove duplicate functions
* GSW-1838 refactor: Using the new constructor to protect raw data in sqrtRatio calculations
* GSW-1838 refactor: need to lock pool.slot0 to prevent re-entry
* refactor: Using clone data to protect original data
* refactor: remove unused import
* fix: sqrtRatio calculation default value issue
* refactor: swap and swap math
* refactor: computeSwapStep
- edge case test (amount : zero and over liquidity)
- EXACT IN / EXACT OUT Case
* refactor: Separate pool transfer-related test code
* refactor: Rename a receiver function param and liquidity math bug fix
- Fix absolute value before checking if delta value is negative to fix handling error issue if negative.
- Rename the param in the receiver function from pool to p.
* refactor: Changed large numbers to const for readability & removed some comments
* refactor: Remove unnecessary function usage
* refactor: Fix error messages to make their meaning clear

---------

Co-authored-by: 0xTopaz <[email protected]>
Co-authored-by: 0xTopaz <[email protected]>
Co-authored-by: Blake <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r3v4s r3v4s r3v4s approved these changes

@dongwon8247 dongwon8247 dongwon8247 approved these changes

@mconcat mconcat Awaiting requested review from mconcat

@onlyhyde onlyhyde Awaiting requested review from onlyhyde

Assignees

@notJoon notJoon

Labels
pool Pool contract related tasks refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@notJoon @onlyhyde @dongwon8247 @r3v4s