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

feat: Connectors v2 message PoC #1292

Merged
merged 35 commits into from
May 4, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
65a3d17
feat: update Message enum
wischli Mar 28, 2023
690430e
feat: add (dummy) Into<u128> for CurrencyId
wischli Mar 28, 2023
b5eb58c
tests: apply changes to existing
wischli Mar 29, 2023
4588b91
refactor: add consts in tests
wischli Mar 29, 2023
0839851
tests: apply new messages
wischli Mar 30, 2023
394fd4e
refactor: apply suggestions from soft review
wischli Mar 30, 2023
6a86151
fix: clippy
wischli Mar 30, 2023
3ba9230
tests: fix integration
wischli Mar 31, 2023
9c84f3a
refactor: add LocalCurrencyId storage
wischli Apr 3, 2023
ccc22b0
Merge remote-tracking branch 'origin/main' into feat/connectors-v2-me…
wischli Apr 19, 2023
af67315
feat: add general index mapping for CurrencyId
wischli Apr 19, 2023
492fdb3
tests: fix integration for connectors
wischli Apr 19, 2023
7726985
fix: clippy warning
wischli Apr 19, 2023
93662a1
refactor: apply suggestions from review
wischli Apr 20, 2023
3db1233
refactor: remove unused code
wischli Apr 20, 2023
6cc8863
refactor: apply latest spec changes
wischli Apr 20, 2023
7cacb2b
refactor: remove CollectFor* msgs
wischli Apr 20, 2023
7f60bd4
tests: cover uts for all message cases
wischli Apr 20, 2023
c064dca
refactor: use TryInto instead of custom GeneralCurrencyIndex trait
wischli Apr 20, 2023
dd2356e
refactor: update message fields
wischli Apr 21, 2023
51542c5
feat: complete GeneralCurrencyIndex
wischli Apr 21, 2023
af3ea8e
style: fix indentation
wischli Apr 21, 2023
572fb34
Apply suggestion from code review by @NunoAlexandre
wischli May 2, 2023
4ecddf9
chore: add message docs, fix order
wischli May 2, 2023
2ab8593
Apply suggestion from review by @offerijns
wischli May 2, 2023
618f7c2
Update pallets/connectors/src/message.rs
wischli May 2, 2023
bdbf65d
docs: improve AssetNotFound
wischli May 2, 2023
14b3f09
docs: apply normalized directionality to Message
wischli May 2, 2023
6a06ba4
Merge branch 'main' into feat/connectors-v2-message-poc
wischli May 3, 2023
d774926
refactor: hashed GeneralCurrencyPrefix
wischli May 4, 2023
05dd755
fix: block local domain transfers
wischli May 4, 2023
6b06a9d
refactor: rm unneeded trait bounding
wischli May 4, 2023
e045d0e
Merge remote-tracking branch 'origin/main' into feat/connectors-v2-me…
wischli May 4, 2023
787cc53
fmt: apply comment wrapping
wischli May 4, 2023
2f22dae
fix: remove new extrinsics
wischli May 4, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions libs/primitives/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,9 @@ pub mod constants {
pub const fn deposit(items: u32, bytes: u32) -> Balance {
items as Balance * 15 * CENTI_CFG + (bytes as Balance) * 6 * CENTI_CFG
}

/// The prefix for tokens managed by Connectors.
pub const GENERAL_CURRENCY_INDEX_PREFIX: [u8; 12] = *b"CfgCnctTknId";
wischli marked this conversation as resolved.
Show resolved Hide resolved
}

/// Listing of parachains we integrate with.
Expand Down
10 changes: 10 additions & 0 deletions libs/traits/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -630,3 +630,13 @@ pub trait TransferAllowance<AccountId> {
currency: Self::CurrencyId,
) -> DispatchResult;
}

/// A trait that enables mapping a currency type to some general index with a distinct prefix.
pub trait GeneralCurrencyIndex<Prefix> {
type CurrencyId;
type GeneralIndex;

/// Returns the `GeneralIndex` of a currency as a concatenation of the generic prefix
/// and its local currency identifier.
fn get_general_index(currency: Self::CurrencyId) -> Result<Self::GeneralIndex, DispatchError>;
}
29 changes: 28 additions & 1 deletion libs/types/src/tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@
// GNU General Public License for more details.

use cfg_primitives::types::{PoolId, TrancheId};
use cfg_traits::TrancheCurrency as TrancheCurrencyT;
use cfg_traits::{GeneralCurrencyIndex, TrancheCurrency as TrancheCurrencyT};
use codec::{Decode, Encode, MaxEncodedLen};
pub use orml_asset_registry::AssetMetadata;
use scale_info::TypeInfo;
#[cfg(feature = "std")]
use serde::{Deserialize, Serialize};
use sp_runtime::{traits::Get, DispatchError, TokenError};

use crate::xcm::XcmMetadata;

Expand Down Expand Up @@ -79,6 +80,29 @@ impl From<StakingCurrency> for CurrencyId {
}
}

impl<Prefix> GeneralCurrencyIndex<Prefix> for CurrencyId
where
Prefix: Get<[u8; 12]>,
{
type CurrencyId = CurrencyId;
type GeneralIndex = u128;

fn get_general_index(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would implement this as a Into / From GeneralCurrencyIndex to/from CurrencyId instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you give a short feedback if c064dca matches your expectations? Then I will add the rest.

currency_id: Self::CurrencyId,
) -> Result<Self::GeneralIndex, DispatchError> {
let mut bytes = [0u8; 16];
bytes[..12].copy_from_slice(&Prefix::get());

let currency_bytes: [u8; 4] = match currency_id {
CurrencyId::ForeignAsset(id32) => Ok(id32.to_be_bytes()),
_ => Err(DispatchError::Token(TokenError::Unsupported)),
}?;

bytes[12..].copy_from_slice(&currency_bytes[..]);
Ok(u128::from_be_bytes(bytes))
}
}

/// A Currency that is solely used by tranches.
///
/// We distinguish here between the enum variant CurrencyId::Tranche(PoolId, TranchId)
Expand Down Expand Up @@ -150,4 +174,7 @@ pub struct CustomMetadata {

/// Whether an asset can be used as a currency to fund Centrifuge Pools.
pub pool_currency: bool,
// TODO: Enable in follow-up PR
// /// The corresponding 20-byte EVM address of the asset.
// pub evm_address: [u8; 20],
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work because this would assume that every token would have to have an evm_address. We could either make it optional, which is still a weak abstraction in my opinion, or we can use the MultiLocation field of the metadata to represent the EVM address of that token, assuming we are registering the asset as it being an asset in a Connector-connected Domain, if that makes sense? By this last part I mean, lets's say that Domain X has a Token X with an EVM address EA; if we register X on the Centrifuge Chain just to register/list a token we handle on Domain X, that approach could work, because when sending/receiving transfers from/to that Domain, we can expect the respective entry in the asset registry to have a MultiLocation holding an EVM address; if it doesn't, we throw an error.

Maybe I am making this sound too complicated, but I am coming from a perspective where every token listed in the asset registry (apart from Tranche tokens) is a potential token that could be transferred through XCM; by using MultiLocation to hold an EVM address that wouldn't fit that purpose, but since that's not the purpose we would be registering the token in the registry in the first place, I think that's fine.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely get your point! This is purely based on the last ConnectorsV2 sync we had were we decided to add evm_address to the metadata of currencies in the asset registry. I am not sure though how to deterministically derive 32 bytes from the flexible MultiLocation which could be quite scaffolded, e.g. Here vs. X2(para, pallet_instance, general_index) etc. How would you handle that?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could use a X2(DomainId, AccountKey20 { network: NetworkId, key: [u8; 20] }}; but that's probably trying to make a shoe out of a shoe box. I remember @offerijns suggesting in that sync to maybe use a type like DomainAddress; the issue with that is that we already have the location field on the Metadata type.

As I am thinking and writing about this, it becomes clearer how there's an abstraction leakage here, in the sense of trying to use a design / abstraction (the asset registry) for something it's not quite designed for. The best would be if we could change the Meadata.location field to be something like enum GlobalLocation { Polkadot(VersionedMultiLocation), EVM(chainId, address) } but we can't do that unless if we change the orml pallet itself.

I have to think a bit more about this 🙃

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use XCM MultiLocations for this, weren't they explicitly designed to support multiple consensus systems, so also able to represent EVM addresses? We can/should look at how Snowbridge intents to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use XCM MultiLocations for this

Yeah that's the discussion above, specifically the X2 + AccountKey20 suggestion.

weren't they explicitly designed to support multiple consensus systems

"Yes" although as we see - at least in XCM v2 - it's not that idiomatic to try and identify an EVM token using MultiLocation. Might need to revisit V3 to hopefully find better support.

We can/should look at how Snowbridge intents to do this.

Absolutely, good point!

}
145 changes: 133 additions & 12 deletions pallets/connectors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,22 @@
#![cfg_attr(not(feature = "std"), no_std)]
use core::convert::TryFrom;

use cfg_traits::PoolInspect;
use cfg_traits::{GeneralCurrencyIndex, PoolInspect};
use cfg_types::domain_address::{Domain, DomainAddress, DomainLocator};
use cfg_utils::{decode_be_bytes, vec_to_fixed_array};
use codec::{Decode, Encode, Input, MaxEncodedLen};
use frame_support::traits::{
fungibles::{Inspect, Mutate, Transfer},
OriginTrait,
use frame_support::{
ensure,
traits::{
fungibles::{Inspect, Mutate, Transfer},
OriginTrait,
},
};
use orml_traits::asset_registry::{self, Inspect as _};
pub use pallet::*;
use scale_info::TypeInfo;
use sp_core::U256;
use sp_runtime::{traits::AtLeast32BitUnsigned, FixedPointNumber};
use sp_runtime::{traits::AtLeast32BitUnsigned, DispatchError, FixedPointNumber};
use sp_std::{convert::TryInto, vec, vec::Vec};
pub mod weights;

Expand Down Expand Up @@ -94,7 +97,7 @@ pub type TrancheIdOf<T> = <<T as Config>::PoolInspect as PoolInspect<
pub type MessageOf<T> =
Message<Domain, PoolIdOf<T>, TrancheIdOf<T>, <T as Config>::Balance, <T as Config>::Rate>;

pub type CurrencyIdOf<T> = <T as pallet_xcm_transactor::Config>::CurrencyId;
pub type CurrencyIdOf<T> = <T as Config>::CurrencyId;

#[frame_support::pallet]
pub mod pallet {
Expand Down Expand Up @@ -162,6 +165,25 @@ pub mod pallet {
Balance = <Self as Config>::Balance,
CustomMetadata = CustomMetadata,
>;

/// The currency type of transferrable token.
type CurrencyId: Parameter
+ Member
+ Copy
+ MaybeSerializeDeserialize
+ Ord
+ TypeInfo
+ MaxEncodedLen
+ Into<<Self as pallet_xcm_transactor::Config>::CurrencyId>
+ GeneralCurrencyIndex<
<Self as Config>::GeneralCurrencyPrefix,
CurrencyId = <Self as Config>::CurrencyId,
GeneralIndex = u128,
>;

/// The prefix for currencies added via Connectors.
#[pallet::constant]
type GeneralCurrencyPrefix: Get<[u8; 12]>;
}

#[pallet::event]
Expand Down Expand Up @@ -196,6 +218,8 @@ pub mod pallet {

#[pallet::error]
pub enum Error<T> {
/// Failed to map the asset to its u128 representation
wischli marked this conversation as resolved.
Show resolved Hide resolved
AssetNotFound,
/// A pool could not be found
PoolNotFound,
/// A tranche could not be found
Expand Down Expand Up @@ -276,6 +300,7 @@ pub mod pallet {
origin: OriginFor<T>,
pool_id: PoolIdOf<T>,
tranche_id: TrancheIdOf<T>,
decimals: u8,
domain: Domain,
) -> DispatchResult {
let who = ensure_signed(origin.clone())?;
Expand All @@ -301,6 +326,7 @@ pub mod pallet {
Message::AddTranche {
pool_id,
tranche_id,
decimals,
token_name,
token_symbol,
price,
Expand Down Expand Up @@ -328,7 +354,7 @@ pub mod pallet {

Self::do_send_message(
who,
Message::UpdateTokenPrice {
Message::UpdateTrancheTokenPrice {
pool_id,
tranche_id,
price,
Expand Down Expand Up @@ -393,7 +419,7 @@ pub mod pallet {
/// Transfer tranche tokens to a given address
#[pallet::weight(< T as Config >::WeightInfo::transfer())]
#[pallet::call_index(6)]
pub fn transfer(
pub fn transfer_tranche_tokens(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Block local domain!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch! 05dd755

origin: OriginFor<T>,
pool_id: PoolIdOf<T>,
tranche_id: TrancheIdOf<T>,
Expand Down Expand Up @@ -427,13 +453,89 @@ pub mod pallet {
)?;

Self::do_send_message(
who,
Message::Transfer {
who.clone(),
Message::TransferTrancheTokens {
pool_id,
tranche_id,
amount,
domain: domain_address.domain(),
address: domain_address.address(),
receiver: domain_address.address(),
sender: who
wischli marked this conversation as resolved.
Show resolved Hide resolved
.encode()
.try_into()
.map_err(|_| DispatchError::Other("Conversion to 32 bytes failed"))?,
wischli marked this conversation as resolved.
Show resolved Hide resolved
},
domain_address.domain(),
)?;

Ok(())
}

/// Transfer non-tranche tokens to a given address
#[pallet::weight(< T as Config >::WeightInfo::transfer())]
#[pallet::call_index(7)]
pub fn transfer(
origin: OriginFor<T>,
asset_id: CurrencyIdOf<T>,
domain_address: DomainAddress,
amount: <T as pallet::Config>::Balance,
) -> DispatchResult {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Block local domain!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch! 05dd755

let who = ensure_signed(origin.clone())?;

ensure!(!amount.is_zero(), Error::<T>::InvalidTransferAmount);

// Transfer to the domain account for bookkeeping
T::Tokens::transfer(
asset_id,
&who,
&DomainLocator::<Domain> {
domain: domain_address.domain(),
}
.into_account_truncating(),
amount,
false,
)?;

Self::do_send_message(
who.clone(),
Message::Transfer {
amount,
currency: <Pallet<T> as GeneralCurrencyIndex<
<T as Config>::GeneralCurrencyPrefix,
>>::get_general_index(asset_id)?,
sender: who
.encode()
.try_into()
.map_err(|_| DispatchError::Other("Conversion to 32 bytes failed"))?,
receiver: domain_address.address(),
},
domain_address.domain(),
)?;

Ok(())
}

/// Add a CurrencyId to the set of known currencies, enabling that currency for Pool creation.
wischli marked this conversation as resolved.
Show resolved Hide resolved
// TODO: Replace weight after benchmarking
#[pallet::weight(< T as Config >::WeightInfo::add_connector())]
#[pallet::call_index(8)]
pub fn add_currency(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have an issue for tracking that. This would be really important!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean the FIXME

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 do not have an existing Github issue but this is part of the asset metadata update which is owned by @NunoAlexandre

Copy link
Contributor Author

@wischli wischli May 4, 2023

Choose a reason for hiding this comment

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

Improved message in 05dd755

origin: OriginFor<T>,
currency_id: CurrencyIdOf<T>,
domain_address: DomainAddress,
) -> DispatchResult {
let who = ensure_signed(origin)?;

let general_index = <Pallet<T> as GeneralCurrencyIndex<
<T as Config>::GeneralCurrencyPrefix,
>>::get_general_index(currency_id)?;

Self::do_send_message(
who,
Message::AddCurrency {
currency: general_index,
// FIXME: In PR which adds evm_address to AssetRegistry
evm_address: [0u8; 20],
},
domain_address.domain(),
)?;
Expand Down Expand Up @@ -491,7 +593,7 @@ pub mod pallet {
fee_payer,
// The currency in which we want to pay fees
CurrencyPayment {
currency: Currency::AsCurrencyId(xcm_domain.fee_currency),
currency: Currency::AsCurrencyId(xcm_domain.fee_currency.into()),
fee_amount: None,
},
// The call to be executed in the destination chain
Expand Down Expand Up @@ -553,6 +655,25 @@ pub mod pallet {
}
}

impl<T, Prefix> GeneralCurrencyIndex<Prefix> for Pallet<T>
where
T: pallet::Config,
{
type CurrencyId = <T as pallet::Config>::CurrencyId;
type GeneralIndex = u128;

fn get_general_index(
currency: <T as pallet::Config>::CurrencyId,
) -> Result<Self::GeneralIndex, DispatchError> {
ensure!(
<T as Config>::AssetRegistry::metadata(&currency).is_some(),
Error::<T>::AssetNotFound
);

CurrencyIdOf::<T>::get_general_index(currency)
}
}

#[cfg(test)]
mod tests {
use cfg_primitives::AccountId;
Expand Down