Skip to content

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

Closed
dobertRowneySr wants to merge 1 commit intoJoystream:luxorfrom
dobertRowneySr:fix/concurrent-amm-revenue-share
Closed

fix: 🐛 amm and revenue shares cannot be concurrent#5125
dobertRowneySr wants to merge 1 commit intoJoystream:luxorfrom
dobertRowneySr:fix/concurrent-amm-revenue-share

Conversation

@dobertRowneySr
Copy link
Copy Markdown
Collaborator

@dobertRowneySr dobertRowneySr commented Apr 4, 2024

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

@dobertRowneySr
Copy link
Copy Markdown
Collaborator Author

Update also the preconditions comment...

Copy link
Copy Markdown
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.

let token_info = Self::ensure_token_exists(token_id)?;
token_info.revenue_split.ensure_inactive::<T>()?;

ensure!(
Copy link
Copy Markdown
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


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

token_data.ensure_can_modify_supply::<T>()?;
Copy link
Copy Markdown
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
Copy Markdown
Member

mnaamani commented Apr 4, 2024

Update also the preconditions comment...

did you forget to push this change maybe?

@mnaamani
Copy link
Copy Markdown
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants