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

Cleaning: Remove TradingPair #1741

Merged
merged 7 commits into from
Feb 23, 2024
Merged

Cleaning: Remove TradingPair #1741

merged 7 commits into from
Feb 23, 2024

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Feb 21, 2024

Description

Removes TradingPair storage and removes all effects on it in the codebase. slack thread

Changes and Descriptions

  • (API change) Remove TradingPair storage from order-book
  • (API change) Remove add_trading_pair() and rm_trading_pair() extrinsics.
  • Remove valid_pair() method from TokenSwaps and Swaps traits.
  • (API change) allow_investment_currency() extrinsic without tranche_id parameter.
  • (API change) disallow_investment_currency() extrinsic without tranche_id parameter.

@lemunozm lemunozm added Q1-easy Can be done by primarily duplicating and adapting code. P5-soon Issue should be addressed soon. D5-breaks-api Pull request changes public api. Document changes publicly. I11-cleaning No mandatory issue that leave the repo more readable/organized labels Feb 21, 2024
@lemunozm lemunozm added this to the Centrifuge 1026 milestone Feb 21, 2024
@lemunozm lemunozm self-assigned this Feb 21, 2024
@lemunozm
Copy link
Contributor Author

lemunozm commented Feb 21, 2024

Ready for review. Blocked by #1737

@lemunozm lemunozm changed the base branch from repo-organization to main February 21, 2024 15:23
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.

Getting cleaner and cleaner here! Looks good except for the call index overwriting.

pallets/order-book/src/lib.rs Show resolved Hide resolved
Copy link
Collaborator

@mustermeiszer mustermeiszer left a 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.

pallets/foreign-investments/src/impls.rs Outdated Show resolved Hide resolved
pallets/foreign-investments/src/impls.rs Outdated Show resolved Hide resolved
pallets/order-book/src/lib.rs Show resolved Hide resolved
Comment on lines -66 to -96
// 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,
>,
Copy link
Collaborator

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense 👍🏻

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree!

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 but misses cleanup of the TradingPair storage. If you want, I can do that.

@lemunozm
Copy link
Contributor Author

Just done here: 1200eaf (tested with running the node with try-runtime)

Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

Good clean up!

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! Thanks again for all the cleanup work you have been doing! 5/5 stars please come back lol

@wischli wischli merged commit 5822bff into main Feb 23, 2024
9 checks passed
@wischli wischli deleted the clear/trading_pair branch February 23, 2024 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D5-breaks-api Pull request changes public api. Document changes publicly. I11-cleaning No mandatory issue that leave the repo more readable/organized P5-soon Issue should be addressed soon. Q1-easy Can be done by primarily duplicating and adapting code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants