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

Implement swap fees #676

Merged
merged 21 commits into from
Jul 12, 2022
Merged

Implement swap fees #676

merged 21 commits into from
Jul 12, 2022

Conversation

maltekliemann
Copy link
Member

@maltekliemann maltekliemann commented Jun 15, 2022 •
edited
Loading

Closes #643.

@maltekliemann maltekliemann added the s:in-progress The pull requests is currently being worked on label Jun 15, 2022
@maltekliemann maltekliemann added this to the v0.3.4 milestone Jun 15, 2022
@maltekliemann maltekliemann self-assigned this Jun 15, 2022
@maltekliemann maltekliemann added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Jun 15, 2022
@maltekliemann maltekliemann requested review from sea212 and Chralt98 and removed request for sea212 June 15, 2022 16:33
Chralt98
Chralt98 reviewed Jun 20, 2022
View reviewed changes
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

This PR extends the pool creation with fee functionality, so that every trade on the pool afterwards charges a fee of a maximum MaxSwapFee amount. Great job!

I wonder where the fee amount goes? To the liquidity providers? Is that already implemented and part of another PR?

zrml/prediction-markets/src/lib.rs Show resolved Hide resolved
primitives/src/constants.rs Show resolved Hide resolved
zrml/prediction-markets/src/tests.rs Show resolved Hide resolved
zrml/swaps/src/lib.rs Show resolved Hide resolved
zrml/swaps/src/lib.rs Outdated Show resolved Hide resolved
zrml/swaps/src/tests.rs Outdated Show resolved Hide resolved
zrml/swaps/src/tests.rs Outdated Show resolved Hide resolved
zrml/swaps/src/tests.rs Outdated Show resolved Hide resolved
zrml/swaps/src/tests.rs Outdated Show resolved Hide resolved
zrml/swaps/src/tests.rs Show resolved Hide resolved
Chralt98
Chralt98 reviewed Jun 21, 2022
View reviewed changes
Copy link
Member

@Chralt98 Chralt98 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 @maltekliemann for the detailed answers. You really nailed it.

zrml/prediction-markets/src/lib.rs Show resolved Hide resolved
sea212
sea212 approved these changes Jul 7, 2022
View reviewed changes
Copy link
Member

@sea212 sea212 left a comment

Choose a reason for hiding this comment

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

Finally another feature that motivates to think before doing the swap 😄 .
Good PR, couldn't find anything critical. Assume this is the usual high standard you provide and the detailed review from @Chralt98 . Good job!

primitives/src/constants.rs Show resolved Hide resolved
zrml/swaps/src/lib.rs Outdated Show resolved Hide resolved
zrml/swaps/src/tests.rs Show resolved Hide resolved
zrml/swaps/src/tests.rs Show resolved Hide resolved
zrml/swaps/src/tests.rs Show resolved Hide resolved
@sea212 sea212 added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Jul 7, 2022
Chralt98
Chralt98 approved these changes Jul 11, 2022
View reviewed changes
Copy link
Member

@Chralt98 Chralt98 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.

Now the swap fee is correctly regarded in the calc_spot_price calculation. You added useful sanity checks, that check if a malicious actor can steal out of the pool. Looks cool.

@maltekliemann maltekliemann merged commit 852ebea into main Jul 12, 2022
@maltekliemann maltekliemann deleted the swap-fees branch July 19, 2022 10:20
@sea212 sea212 added i:spec-changed ⚠️ Implies change in spec version i:transactions-changed ⚠️ Implies change in transaction version labels Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sea212 sea212 sea212 approved these changes

@Chralt98 Chralt98 Chralt98 approved these changes

Assignees

@maltekliemann maltekliemann

Labels
i:spec-changed ⚠️ Implies change in spec version i:transactions-changed ⚠️ Implies change in transaction version s:accepted This pull request is ready for merge
Projects
None yet
Milestone
v0.3.5
Development

Successfully merging this pull request may close these issues.

[swaps] Implement trading fees
3 participants
@maltekliemann @sea212 @Chralt98