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

fix: 🐛 amm and revenue shares cannot be concurrent #5125

Closed
wants to merge 1 commit into from
Closed

fix: 🐛 amm and revenue shares cannot be concurrent #5125

wants to merge 1 commit into from

Conversation

ignazio-bovo
Copy link
Contributor

@ignazio-bovo ignazio-bovo commented Apr 4, 2024 •
edited
Loading

Goals

  1. Amm cannot be activate when a revenue share is active
  2. Revenue Share cannot be issued when Amm is active

Changes

  • Added unit test for constraint 1
  • Added unit test for constraint 2
  • Added logic to the code to satisfy constraints above
  • Added extra test to highlight that a token sale can still be issued during revenue share
  • regenerate metadata and types

Notes

  • No migrations needed to accommodate changes

@ignazio-bovo
Copy link
Contributor Author

Update also the preconditions comment...

mnaamani
mnaamani requested changes Apr 4, 2024
View reviewed changes
Copy link
Member

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

Minor update to comments, otherwise looks good to go.

runtime-modules/project-token/src/lib.rs
@@ -1500,6 +1500,11 @@ impl<T: Config>
let token_info = Self::ensure_token_exists(token_id)?;
token_info.revenue_split.ensure_inactive::<T>()?;

ensure!(
Copy link
Member

Choose a reason for hiding this comment

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

please add this precondition to the code comments

runtime-modules/project-token/src/lib.rs
@@ -1654,6 +1659,8 @@ impl<T: Config>

let token_data = Self::ensure_token_exists(token_id)?;

token_data.ensure_can_modify_supply::<T>()?;
Copy link
Member

Choose a reason for hiding this comment

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

Please add this pre-condition to the code comments as well.

@mnaamani
Copy link
Member

mnaamani commented Apr 4, 2024

Update also the preconditions comment...

did you forget to push this change maybe?

@mnaamani mnaamani mentioned this pull request Apr 5, 2024
Merged
@mnaamani
Copy link
Member

mnaamani commented Apr 5, 2024

This solution works but "it would force creators to open and close their AMMs frequently which is not a good idea"
So replaced by #5127

@mnaamani mnaamani closed this Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnaamani mnaamani mnaamani requested changes

@freakstatic freakstatic Awaiting requested review from freakstatic

@kdembler kdembler Awaiting requested review from kdembler

Assignees
No one assigned
Labels
luxor-network
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ignazio-bovo @mnaamani