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

AMM: Swap and add liquidity scripts #312

Merged
merged 44 commits into from
Jan 9, 2023
Merged

Conversation

supiket
Copy link
Contributor

@supiket supiket commented Dec 2, 2022

Type of change

  • New feature

Changes

  • Swap exact input and swap exact output scripts
  • Atomic deposit and add liquidity script
  • Test utils module
  • Tests for scripts

Notes

Before merging to master:

  • Script tests for revert cases should have the actual error messages instead of #[should_panic(expected = "Revert(18446744073709486080)")] once feat: support logs from external contracts is merged
  • fuels dependencies should be changed to version instead of master branch once new version is out
  • exchange-contract/utils/multiply_divide function should be renamed to something along the lines of "proportions"

Related Issues

Closes #229
Closes #287

@supiket supiket added New Feature New addition that does not currently exist App: AMM Label used to filter for the app issue labels Dec 2, 2022
@supiket supiket self-assigned this Dec 2, 2022
@supiket supiket changed the title AMM: Swap exact input script AMM: Swap exact input and swap exact output scripts Dec 7, 2022
@supiket
Copy link
Contributor Author

supiket commented Dec 8, 2022

@Braqzen you can take a look at the test-utils :)

@supiket supiket changed the title AMM: Swap exact input and swap exact output scripts AMM: Swap and add liquidity scripts Dec 10, 2022
@simonr0204 simonr0204 self-requested a review January 3, 2023 19:36
simonr0204
simonr0204 previously approved these changes Jan 3, 2023
@supiket supiket changed the base branch from master to staging January 4, 2023 15:02
@supiket supiket requested review from Braqzen and simonr0204 January 4, 2023 15:03
@simonr0204
Copy link
Contributor

Any extra thoughts here @Braqzen or can we click the big green button?

@Braqzen
Copy link
Contributor

Braqzen commented Jan 6, 2023

Any extra thoughts here @Braqzen or can we click the big green button?

Sorry, I was planning on reviewing this tomorrow. Will get around to this soon

Copy link
Contributor

@Braqzen Braqzen left a comment

Choose a reason for hiding this comment

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

I have not carefully reviewed the tests to see if the logic covers all cases or if they make sense - via this review.

AMM/project/contracts/AMM-contract/Cargo.toml Show resolved Hide resolved
AMM/project/contracts/exchange-contract/src/events.sw Outdated Show resolved Hide resolved
AMM/project/libraries/src/data_structures.sw Outdated Show resolved Hide resolved
@@ -54,22 +49,18 @@ pub fn minimum_output_given_exact_input(
result_wrapped.unwrap()
}

pub fn multiply_div(a: u64, b: u64, c: u64) -> u64 {
pub fn multiply_divide(a: u64, b: u64, c: u64) -> u64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of this function doesn't really convey much as to what it is trying to accomplish. I would rename and potentially add some documentation to outline what is the multiplication and division achieving.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot come up with a better name now, will rename by the time this is merged to master.

AMM/project/contracts/exchange-contract/src/main.sw Outdated Show resolved Hide resolved
AMM/project/scripts/swap-exact-input/tests/harness.rs Outdated Show resolved Hide resolved
@supiket supiket merged commit d9ef3ec into staging Jan 9, 2023
@supiket supiket deleted the supiket/swap-route-script branch January 9, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App: AMM Label used to filter for the app issue New Feature New addition that does not currently exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AMM: Atomic deposit and add liquidity script AMM: Swap exact input and swap exact output scripts
4 participants