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

feat: add wrappers for dynamically rebased tokens #935

Closed
wants to merge 1 commit into from

Conversation

gregdhill
Copy link
Member

@gregdhill gregdhill commented Feb 16, 2023

Adds wrappers inspired by those in orml-tokens (since they don't implement the MultiCurrency trait) to dynamically rebase tokens if a pairing exists in the Oracle pallet.

Closes #926

Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Copy link
Member

@sander2 sander2 left a comment

Choose a reason for hiding this comment

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

Closes #926

I don't think it does? It adds some, but not all, of the required code, it seems to me? Rather than merging this as-is, I think I'd like this pr to include the rest of the required changes since it'll also be easier to review that way. That is, instead of having multiple PRs, having a single PR with multiple commits

}
}

pub struct Combiner<AccountId, TestKey, A, B>(PhantomData<(AccountId, TestKey, A, B)>);
Copy link
Member

Choose a reason for hiding this comment

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

could you add some docstrings to each of these structs explaining what they do?

@gregdhill
Copy link
Member Author

Closes #926

I don't think it does?

Well technically we can close that issue anyway since it is not possible to support imbalanced assets using Curve v1.

It adds some, but not all, of the required code, it seems to me?

No I think it does, the requirement is that we can link assets dynamically (in the oracle pallet) to convert to some other asset and back again which it does. The only thing missing is to consume these wrappers in the runtime code.

@sander2
Copy link
Member

sander2 commented Feb 28, 2023

No I think it does, the requirement is that we can link assets dynamically (in the oracle pallet) to convert to some other asset and back again which it does. The only thing missing is to consume these wrappers in the runtime code.

Can you add this, and also some tests to show that it's working?

@gregdhill
Copy link
Member Author

This implementation is not correct, we need to re-balance the pool anytime we would interact with it - not just on deposit / withdrawal. See https://github.com/nutsfinance/stable-asset/blob/82577aefff91afb7c6df00d4193e4c61b4c9c1c1/lib/stable-asset/src/lib.rs#L1326

@gregdhill
Copy link
Member Author

Succeeded by #1063

@gregdhill gregdhill closed this Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stable AMM doesn't support price imbalance
2 participants