-
Notifications
You must be signed in to change notification settings - Fork 88
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
Cleaning: Remove TradingPair
#1741
Conversation
Ready for review. Blocked by #1737 |
8d9b018
to
40c7bbe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting cleaner and cleaner here! Looks good except for the call index overwriting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good mostly. The FI logic is not correct IIUC. OTherwise good to go.
// Register trading pairs one by one | ||
runtime_common::migrations::local_currency::add_bidirectional_trading_pair::Migration< | ||
super::Runtime, | ||
UsdcDot, | ||
LocalCurrencyIdUsdc, | ||
MinOrderAmount, | ||
>, | ||
runtime_common::migrations::local_currency::add_bidirectional_trading_pair::Migration< | ||
super::Runtime, | ||
UsdcEth, | ||
LocalCurrencyIdUsdc, | ||
MinOrderAmount, | ||
>, | ||
runtime_common::migrations::local_currency::add_bidirectional_trading_pair::Migration< | ||
super::Runtime, | ||
UsdcBase, | ||
LocalCurrencyIdUsdc, | ||
MinOrderAmount, | ||
>, | ||
runtime_common::migrations::local_currency::add_bidirectional_trading_pair::Migration< | ||
super::Runtime, | ||
UsdcArb, | ||
LocalCurrencyIdUsdc, | ||
MinOrderAmount, | ||
>, | ||
runtime_common::migrations::local_currency::add_bidirectional_trading_pair::Migration< | ||
super::Runtime, | ||
UsdcCelo, | ||
LocalCurrencyIdUsdc, | ||
MinOrderAmount, | ||
>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need a migration to kill the storage or trading pairs... WDYT @wischli ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we should do the same for AssetPairOrders
already removed from the previous PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After another thought, I think it's safer to just reset completely pallet-order-book
. We should close, of course, the current pending order before the RU.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but misses cleanup of the TradingPair
storage. If you want, I can do that.
Just done here: 1200eaf (tested with running the node with try-runtime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good clean up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks again for all the cleanup work you have been doing! 5/5 stars please come back lol
Description
Removes
TradingPair
storage and removes all effects on it in the codebase. slack threadChanges and Descriptions
TradingPair
storage fromorder-book
add_trading_pair()
andrm_trading_pair()
extrinsics.valid_pair()
method fromTokenSwaps
andSwaps
traits.allow_investment_currency()
extrinsic withouttranche_id
parameter.disallow_investment_currency()
extrinsic withouttranche_id
parameter.