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

Fix: order book working with local currencies #1773

Merged
merged 6 commits into from
Mar 18, 2024
Merged

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Mar 18, 2024

Description

  • Remove DecimalConverter trait, which does not handle Local currencies.
  • Compute the min fulfillment amount directly in order-book generically for any currency.

@lemunozm lemunozm added I2-bug The code fails to follow expected behaviour. D0-ready Pull request can be merged without special precaution and notification. P9-drop-everything Everyone should address this issue now. labels Mar 18, 2024
@lemunozm lemunozm self-assigned this Mar 18, 2024
Comment on lines 671 to 684
let from_decimals = T::AssetRegistry::metadata(&T::NativeCurrency::get())
.ok_or(Error::<T>::InvalidCurrencyId)?
.decimals;

let to_decimals = T::AssetRegistry::metadata(&currency)
.ok_or(Error::<T>::InvalidCurrencyId)?
.decimals;

Ok(convert_balance_decimals(
from_decimals,
to_decimals,
T::MinFulfillmentAmountNative::get().into(),
)?
.into())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can avoid having an external trait to do this if we know the Native currency

Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

LGTM except for the two debugging leftovers. Thanks a lot for taking care of this so quickly! I propose to open a follow up PR which bumps spec versions.

pallets/order-book/src/lib.rs Outdated Show resolved Hide resolved
wischli
wischli previously approved these changes Mar 18, 2024
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Faster than Usain Bolt!

@lemunozm
Copy link
Contributor Author

😆, thanks for reviewing William!

@lemunozm lemunozm enabled auto-merge (squash) March 18, 2024 09:25
@lemunozm
Copy link
Contributor Author

Not so fast, benchmarking is failing

@lemunozm
Copy link
Contributor Author

Modified the fix a bit to use the native decimals instead of the native currency itself, to avoid difficult initializations of the native currency in benchmarking of pallets that uses order-book

@lemunozm lemunozm merged commit 5dfdbba into main Mar 18, 2024
9 checks passed
@lemunozm lemunozm deleted the fix/decimal-convert-local branch March 18, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D0-ready Pull request can be merged without special precaution and notification. I2-bug The code fails to follow expected behaviour. P9-drop-everything Everyone should address this issue now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants