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

Abstraction of the base asset #629

Merged
merged 31 commits into from
Jul 13, 2022
Merged

Conversation

Chralt98
Copy link
Member

Related to #100.

The aim is to use only the MultiCurrency trait for the prediction market pallet instead of directly calling the balances pallet.

Therefore it's easier to use a different base asset (like aUSD) later.

@Chralt98 Chralt98 added the s:in-progress The pull requests is currently being worked on label May 16, 2022
@Chralt98 Chralt98 requested review from sea212 and maltekliemann May 16, 2022 10:30
@Chralt98 Chralt98 self-assigned this May 16, 2022
@Chralt98 Chralt98 linked an issue May 16, 2022 that may be closed by this pull request
@maltekliemann maltekliemann added this to the v0.3.3 milestone May 16, 2022
@Chralt98
Copy link
Member Author

In the runtime the only occurrence of the orml_currencies pallet is the swaps pallet here.

I would assume, that the correct abstraction of the base asset is to use the orml_currencies as Shares here in the prediction market pallet. As I understand it correctly the orml_tokens lets us to handle multiple currencies (for us outcome tokens / assets) and the orml_currencies lets us conveniently handle all the functionality of orml_tokens plus the functionality to manage the NativeCurrency with the BasicCurrencyAdapter.

But there is a problem, that the new NamedMultiReservableCurrency trait isn't allowed to use as a MultiCurrency in the orml_currencies pallet yet. I opened an issue for that here.

@Chralt98
Copy link
Member Author

At the moment I use T::BaseAsset::get() which is initialised with the GetNativeCurrency at the moment. But it's later much easier to enter it in each extrinsic as a call parameter.

@maltekliemann maltekliemann modified the milestones: v0.3.3, v0.3.4 May 24, 2022
@Chralt98 Chralt98 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels May 24, 2022
@Chralt98 Chralt98 marked this pull request as ready for review May 24, 2022 20:57
@Chralt98 Chralt98 requested a review from lsaether June 7, 2022 10:26
Copy link
Contributor

@maltekliemann maltekliemann left a comment

Choose a reason for hiding this comment

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

Good stuff! 👍

I guess the next step is to add a base_asset field to the Market primitive and replace ZTG with the market's base_asset where appropriate?

@maltekliemann maltekliemann added s:revision-needed The pull requests must be revised and removed s:review-needed The pull request requires reviews labels Jun 10, 2022
@Chralt98 Chralt98 added s:review-needed The pull request requires reviews and removed s:revision-needed The pull requests must be revised labels Jun 13, 2022
@Chralt98 Chralt98 requested a review from maltekliemann June 13, 2022 10:31
Copy link
Contributor

@maltekliemann maltekliemann left a comment

Choose a reason for hiding this comment

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

👍

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.

Looks great overall, thanks for paving the way to using other chains assets within the prediction markets. Can't await the moment when the first external token enters a market!

Comment on lines -1129 to -1044
/// Slash
type Slash: OnUnbalanced<NegativeImbalanceOf<Self>>;

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with that, we'll handle treasury deposits a bit later down the line.

@sea212
Copy link
Member

sea212 commented Jul 7, 2022

I'll have a look and approve eventually after overflow handling was applied and main was merge into this.

@Chralt98 Chralt98 requested a review from sea212 July 12, 2022 10:03
@sea212 sea212 added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Jul 12, 2022
@Chralt98 Chralt98 merged commit 61e1beb into main Jul 13, 2022
@Chralt98 Chralt98 deleted the chralt98-multi-currency-base-asset branch July 19, 2022 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:accepted This pull request is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use MultiCurrency in PredictionMarkets to abstract the base asset
3 participants