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

refactor: registry traits consolidation #701

Merged

Conversation

martinfridrich
Copy link
Contributor

Description

This PR consolidates registry traits and refactors all the pallets/code to use new traits and removes old registry traits.

Copy link

Crate versions that have not been updated:

  • runtime-integration-tests: v1.14.0
  • pallet-asset-registry: v3.0.0
  • pallet-circuit-breaker: v1.1.16
  • pallet-lbp: v4.6.16
  • pallet-staking: v2.0.0
  • pallet-xcm-rate-limiter: v0.1.1
  • hydradx-adapters: v0.6.3
  • hydradx-runtime: v184.0.0

Crate versions that have been updated:

  • pallet-bonds: v2.0.0 -> v2.1.0
  • pallet-dca: v1.2.0 -> v1.2.1
  • pallet-liquidity-mining: v4.2.4 -> v4.3.0
  • pallet-omnipool: v3.2.3 -> v3.3.0
  • pallet-omnipool-liquidity-mining: v2.0.11 -> v2.1.0
  • pallet-otc: v1.0.2 -> v1.1.0
  • pallet-stableswap: v3.3.1 -> v3.4.0
  • pallet-xyk: v6.3.0 -> v6.4.0
  • hydradx-traits: v2.7.0 -> v3.0.0

Runtime version has not been increased.

Token,
PoolShare(AssetId, AssetId), // Use XYX instead
Copy link
Contributor

Choose a reason for hiding this comment

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

this would require migration in basislisk ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@@ -128,6 +131,9 @@ pub mod pallet {
#[pallet::constant]
type FeeReceiver: Get<Self::AccountId>;

/// Asset location type
type AssetLocation: Parameter + Member + Default + MaxEncodedLen;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here as in xyk

Copy link
Contributor

@apopiak apopiak left a comment

Choose a reason for hiding this comment

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

Looks fine to me

Comment on lines 724 to 727
let bounded_name = match Self::try_into_bounded(Some(name.to_vec()))? {
Some(n) => n,
None => return Err(Error::<T>::InvalidAssetname.into()),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let bounded_name = match Self::try_into_bounded(Some(name.to_vec()))? {
Some(n) => n,
None => return Err(Error::<T>::InvalidAssetname.into()),
};
let bounded_name = Self::try_into_bounded(Some(name.to_vec())).map_err(|_| Error::<T>::InvalidAssetname)?;

Shouldn't need the match, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this would suppress error returned from try_into_bounded().
InvalidAssetname is something that should never happen. try_into_bounded returns None only if None was provided as param.

@@ -36,6 +36,7 @@ use sp_runtime::{
use std::cell::RefCell;
use std::collections::HashMap;
use std::marker::PhantomData;
//use frame_system::GenesisConfig;
Copy link
Contributor

Choose a reason for hiding this comment

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

commented code

@@ -0,0 +1,67 @@
// This file is part of Basilisk-node.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// This file is part of Basilisk-node.
// This file is part of HydraDX-node.

use serde::{Deserialize, Serialize};

/// Asset Pair representation for AMM trades
/// ( asset_a, asset_b ) combination where asset_a is meant to be exchanged for asset_b
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// ( asset_a, asset_b ) combination where asset_a is meant to be exchanged for asset_b
/// `( asset_in, asset_out )` combination where `asset_in` is meant to be exchanged for `asset_out`

Comment on lines 32 to 33
/// asset_in represents asset coming into the pool
/// asset_out represents asset coming out of the pool
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// asset_in represents asset coming into the pool
/// asset_out represents asset coming out of the pool
/// `asset_in` represents asset coming into the pool
/// `asset_out` represents asset coming out of the pool

@martinfridrich martinfridrich merged commit f88cc0b into feat/permissionless-asset-registry Nov 22, 2023
3 of 4 checks passed
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.

3 participants