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: Unify DomainAddress and account conversions #1967

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 72 additions & 65 deletions libs/types/src/domain_address.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,113 +10,120 @@
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

use cfg_utils::vec_to_fixed_array;
use frame_support::pallet_prelude::RuntimeDebug;
use parity_scale_codec::{Decode, Encode, MaxEncodedLen};
use scale_info::TypeInfo;
use sp_runtime::traits::AccountIdConversion;
use sp_runtime::{traits::AccountIdConversion, TypeId};

use crate::EVMChainId;

const MAX_ADDRESS_SIZE: usize = 32;
pub type LocalAddress = [u8; 32];
pub type EthAddress = [u8; 20];

pub fn local_to_eth_address(address: LocalAddress) -> EthAddress {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Let's stick to one namespace for Eth, Evm, Solidity

Suggested change
pub fn local_to_eth_address(address: LocalAddress) -> EthAddress {
pub fn local_to_evm_address(address: LocalAddress) -> EthAddress {

Copy link
Contributor Author

@lemunozm lemunozm Aug 15, 2024

Choose a reason for hiding this comment

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

I wanted to distinguish for the name:

  • eth: just the address
  • evm: and ethereum address + chain information

I say this because not sure for example if [u8; 20] is an evm address, I think not because it does not have chain information 🤔

But not sure if this association makes sense TBH. It seems not 😅

Copy link
Contributor

@wischli wischli Aug 15, 2024

Choose a reason for hiding this comment

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

I definitely get what you are saying but to me, having two namespaces just causes confusion.

However, since he address format is referred to as the Ethereum address format (as least from Metamask), it appears to me your distinction is correct. So fine with keeping as is.

Copy link
Contributor Author

@lemunozm lemunozm Aug 15, 2024

Choose a reason for hiding this comment

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

Will add a comment regarding this then to explain the why. Thanks for the ref.

I think the key sentence is: in a EVM there are ETH address, so evm makes sense for domains, and eth for addresses

Copy link
Contributor

Choose a reason for hiding this comment

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

Frontier's approach is pretty straightforward when it comes to eth/evm addresses, they name these AccountId20 - here.

As someone who didn't have to deal much with Domain/DomainAddress, I find that local/eth/evm are confusing as well tbh.

Why not go for something simpler like:

  • CentrifugeAccount - AccountId32.
  • EvmAccount - AccountId20/[u8;20].
  • CentrifugeEvmAccount - AccountId32 that's basically EvmAccount + chain_id + tag.

Then we could have implementations for:

  • CentrifugeAccount -> EvmAccount - truncate;
  • EvmAccount -> CentrifugeAccount - pad with 0s;
  • CentrifugeAccount <-> CentrifugeEvmAccount - no need;
  • EvmAccount -> CentrifugeEvmAccount - append chain_id + tag;
  • CentrifugeEvmAccount -> EvmAccount - truncate;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for sharing this!

And these CentrifugeAccount, etc, should be types on their own, right? So you would directly remove the DomainAddress type? Which type will you use to track all of them generically?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I would add them to the DomainAddress:

pub enum DomainAddress {
	/// A standard Centrifuge account.
	Centrifuge(CentrifugeAccount),
	
	/// A Centrifuge account derived from an `EvmAccount`.
	CentrifugeEvm(CentrifugeEvmAccount),
	
	/// An EVM-compatible account.
	Evm(EvmAccount),
}

This way we can keep the above mentioned conversions, and just have From implementations for DomainAddress, and for each of these account types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I like the way you're taking it.

Just a problem with this. The variant Evm(EvmAccount) will not have a Domain information associated with it. A DomainAddress requires it to later extract this.

For sure, I would not have time to do such a refactor for Monday 😅

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 add a struct for the evm account that has the chain and the actual account, or keep it as it is right now? I dunno, just thinking out loud at this point ^^

Copy link
Contributor Author

@lemunozm lemunozm Aug 15, 2024

Choose a reason for hiding this comment

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

But that would be what we have right now, right? 🤔

I'm going to sketch a new draft with some ideas from this, but reusing the current DomainAddress, let's see. Thanks for your thoughts!

*address
.split_first_chunk::<20>()
.expect("always fit, qed")
.0
}

pub fn evm_to_local_address(chain_id: u64, address: EthAddress) -> LocalAddress {
// We use a custom encoding here rather than relying on
// `AccountIdConversion` for a couple of reasons:
// 1. We have very few bytes to spare, so choosing our own fields is nice
// 2. AccountIdConversion puts the tag first, which can unbalance the storage
// trees if users create many H160-derived accounts. We put the tag last
// here.
let tag = b"EVM";
let mut bytes = [0; 32];
bytes[0..20].copy_from_slice(&address);
bytes[20..28].copy_from_slice(&chain_id.to_be_bytes());
bytes[28..31].copy_from_slice(tag);
bytes
}

/// A Domain is a chain or network we can send a message to.
#[derive(Encode, Decode, Clone, Eq, MaxEncodedLen, PartialEq, RuntimeDebug, TypeInfo)]
#[derive(Encode, Decode, Clone, Copy, Eq, MaxEncodedLen, PartialEq, RuntimeDebug, TypeInfo)]
pub enum Domain {
/// Referring to the Centrifuge Parachain. Will be used for handling
/// incoming messages.
///
/// NOTE: messages CAN NOT be sent directly from the Centrifuge chain to the
/// Centrifuge chain itself.
Centrifuge,
/// Referring to the Local Parachain.
Local,
lemunozm marked this conversation as resolved.
Show resolved Hide resolved
/// An EVM domain, identified by its EVM Chain Id
EVM(EVMChainId),
Evm(EVMChainId),
}

impl TypeId for Domain {
const TYPE_ID: [u8; 4] = crate::ids::DOMAIN_ID;
}

impl Domain {
pub fn into_account<AccountId: Encode + Decode>(&self) -> AccountId {
DomainLocator {
domain: self.clone(),
}
.into_account_truncating()
self.into_account_truncating()
}
}

#[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)]
pub struct DomainLocator<Domain> {
pub domain: Domain,
}

#[derive(Encode, Decode, Clone, Eq, MaxEncodedLen, PartialEq, RuntimeDebug, TypeInfo)]
pub enum DomainAddress {
/// A Centrifuge-Chain based account address, 32-bytes long
Centrifuge([u8; 32]),
/// An EVM chain address, 20-bytes long
EVM(EVMChainId, [u8; 20]),
/// A local based account address
Local(LocalAddress),
/// An EVM chain address
Evm(EVMChainId, EthAddress),
}

impl Default for DomainAddress {
fn default() -> Self {
DomainAddress::Centrifuge([0; 32])
}
impl TypeId for DomainAddress {
const TYPE_ID: [u8; 4] = crate::ids::DOMAIN_ADDRESS_ID;
}

impl DomainAddress {
pub fn evm(chain_id: EVMChainId, address: [u8; 20]) -> Self {
Self::EVM(chain_id, address)
}

pub fn centrifuge(address: [u8; 32]) -> Self {
Self::Centrifuge(address)
}
}

impl From<(EVMChainId, [u8; 20])> for DomainAddress {
fn from((chain_id, address): (EVMChainId, [u8; 20])) -> Self {
Self::evm(chain_id, address)
impl Default for DomainAddress {
fn default() -> Self {
DomainAddress::Local(LocalAddress::default())
}
}

impl From<DomainAddress> for Domain {
fn from(x: DomainAddress) -> Self {
match x {
DomainAddress::Centrifuge(_) => Domain::Centrifuge,
DomainAddress::EVM(chain_id, _) => Domain::EVM(chain_id),
DomainAddress::Local(_) => Domain::Local,
DomainAddress::Evm(chain_id, _) => Domain::Evm(chain_id),
}
}
}

impl DomainAddress {
/// Get the address in a 32-byte long representation.
/// For EVM addresses, append 12 zeros.
pub fn address(&self) -> [u8; 32] {
match self.clone() {
Self::Centrifuge(x) => x,
Self::EVM(_, x) => vec_to_fixed_array(x),
pub fn new(domain: Domain, address: [u8; MAX_ADDRESS_SIZE]) -> Self {
match domain {
Domain::Local => DomainAddress::Local(LocalAddress::from(address)),
Domain::Evm(chain_id) => DomainAddress::Evm(chain_id, local_to_eth_address(address)),
}
}

pub fn from_local(address: impl Into<LocalAddress>) -> DomainAddress {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I am not sure I am fully happy with the naming here because the returned domain address implicitly local even though it could also be EVM from a 32 byte array input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the user uses a 32-byte array (that represents an eth address expanded to 32 bytes) and sets this as local, then they are using the method wrongly. Or if not wrongly, they are creating an account from an array with X data, where X is a eth address in the array.

Not sure if I got what you want to say.

DomainAddress::Local(address.into())
}

pub fn from_evm(chain_id: EVMChainId, address: impl Into<EthAddress>) -> DomainAddress {
DomainAddress::Evm(chain_id, address.into())
}

pub fn domain(&self) -> Domain {
self.clone().into()
}
}

#[cfg(test)]
mod tests {
use parity_scale_codec::{Decode, Encode};

use super::*;

#[test]
fn test_domain_encode_decode() {
test_domain_identity(Domain::Centrifuge);
test_domain_identity(Domain::EVM(1284));
test_domain_identity(Domain::EVM(1));
impl DomainAddress {
pub fn as_local<Address: From<LocalAddress>>(&self) -> Address {
match self.clone() {
Self::Local(x) => x,
Self::Evm(chain_id, x) => evm_to_local_address(chain_id, x),
}
.into()
}

/// Test that (decode . encode) results in the original value
fn test_domain_identity(domain: Domain) {
let encoded = domain.encode();
let decoded = Domain::decode(&mut encoded.as_slice()).unwrap();

assert_eq!(domain, decoded);
pub fn as_eth<Address: From<EthAddress>>(&self) -> Address {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Namespace

Suggested change
pub fn as_eth<Address: From<EthAddress>>(&self) -> Address {
pub fn as_evm<Address: From<EthAddress>>(&self) -> Address {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking now about just eth/evm, WDYT?

match self.clone() {
Self::Local(x) => local_to_eth_address(x),
Self::Evm(_, x) => x,
}
.into()
}
}
14 changes: 3 additions & 11 deletions libs/types/src/ids.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,7 @@
use frame_support::PalletId;
use sp_runtime::TypeId;

use crate::{
domain_address::{DomainAddress, DomainLocator},
investments::InvestmentAccount,
};
use crate::investments::InvestmentAccount;

// The TypeId impl we derive pool-accounts from
impl<InvestmentId> TypeId for InvestmentAccount<InvestmentId> {
Expand All @@ -45,10 +42,5 @@ pub const CHAIN_BRIDGE_NATIVE_TOKEN_ID: [u8; 4] = *b"xCFG";
/// The identifier of the group eligible to receive block rewards.
pub const COLLATOR_GROUP_ID: u32 = 1;

impl TypeId for DomainAddress {
const TYPE_ID: [u8; 4] = *b"dadr";
}

impl<Domain> TypeId for DomainLocator<Domain> {
const TYPE_ID: [u8; 4] = *b"domn";
}
pub const DOMAIN_ID: [u8; 4] = *b"dadr";
pub const DOMAIN_ADDRESS_ID: [u8; 4] = *b"domn";
14 changes: 7 additions & 7 deletions node/src/chain_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ use cfg_primitives::{
SAFE_XCM_VERSION,
};
use cfg_types::{
domain_address::DomainAddress,
fee_keys::FeeKey,
tokens::{usdc, AssetMetadata, CrossChainTransferability, CurrencyId, CustomMetadata},
};
use cfg_utils::vec_to_fixed_array;
use cumulus_primitives_core::ParaId;
use hex_literal::hex;
use runtime_common::account_conversion::AccountConverter;
use sc_chain_spec::{ChainSpecExtension, ChainSpecGroup};
use sc_service::{ChainType, Properties};
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -271,8 +271,8 @@ fn centrifuge_genesis(
let chain_id: u32 = id.into();

endowed_accounts.extend(endowed_evm_accounts.into_iter().map(|(addr, id)| {
let chain_id = id.unwrap_or_else(|| chain_id.into());
AccountConverter::convert_evm_address(chain_id, addr)
let chain_id = id.unwrap_or(chain_id.into());
DomainAddress::from_evm(chain_id, addr).as_local()
}));

let num_endowed_accounts = endowed_accounts.len();
Expand Down Expand Up @@ -371,8 +371,8 @@ fn altair_genesis(
let chain_id: u32 = id.into();

endowed_accounts.extend(endowed_evm_accounts.into_iter().map(|(addr, id)| {
let chain_id = id.unwrap_or_else(|| chain_id.into());
AccountConverter::convert_evm_address(chain_id, addr)
let chain_id = id.unwrap_or(chain_id.into());
DomainAddress::from_evm(chain_id, addr).as_local()
}));

let num_endowed_accounts = endowed_accounts.len();
Expand Down Expand Up @@ -472,8 +472,8 @@ fn development_genesis(
let chain_id: u32 = id.into();

endowed_accounts.extend(endowed_evm_accounts.into_iter().map(|(addr, id)| {
let chain_id = id.unwrap_or_else(|| chain_id.into());
AccountConverter::convert_evm_address(chain_id, addr)
let chain_id = id.unwrap_or(chain_id.into());
DomainAddress::from_evm(chain_id, addr).as_local()
}));

let num_endowed_accounts = endowed_accounts.len();
Expand Down
6 changes: 3 additions & 3 deletions pallets/axelar-router/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ pub mod pallet {

T::Receiver::receive(
AxelarId::Evm(chain_id).into(),
DomainAddress::EVM(chain_id, source_address_bytes),
DomainAddress::Evm(chain_id, source_address_bytes),
payload.to_vec(),
)
}
Expand Down Expand Up @@ -324,7 +324,7 @@ pub mod pallet {

match config.domain {
DomainConfig::Evm(evm_config) => {
let sender_evm_address = H160::from_slice(&origin.address()[0..20]);
let sender_eth_address = origin.as_eth::<H160>();

let message = wrap_into_axelar_msg(
message,
Expand All @@ -334,7 +334,7 @@ pub mod pallet {
.map_err(DispatchError::Other)?;

T::Transactor::call(
sender_evm_address,
sender_eth_address,
evm_config.target_contract_address,
message.as_slice(),
evm_config.fee_values.value,
Expand Down
6 changes: 3 additions & 3 deletions pallets/axelar-router/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const LP_CONTRACT_ADDRESS: H160 = H160::repeat_byte(1);
const AXELAR_CONTRACT_ADDRESS: H160 = H160::repeat_byte(2);
const SOURCE_ADDRESS: H160 = H160::repeat_byte(3);
const AXELAR_CONTRACT_HASH: H256 = H256::repeat_byte(42);
const SENDER: DomainAddress = DomainAddress::Centrifuge([0; 32]);
const SENDER: DomainAddress = DomainAddress::Local([0; 32]);
const MESSAGE: &[u8] = &[1, 2, 3];
const FEE_VALUE: U256 = U256::zero();
const GAS_LIMIT: U256 = U256::one();
Expand Down Expand Up @@ -91,7 +91,7 @@ mod send {
correct_configuration();

Transactor::mock_call(move |from, to, data, value, gas_price, gas_limit| {
assert_eq!(from, H160::from_slice(&SENDER.address()[0..20]));
assert_eq!(from, SENDER.as_eth());
assert_eq!(to, AXELAR_CONTRACT_ADDRESS);
assert_eq!(data, &wrap_message(MESSAGE.to_vec()));
assert_eq!(value, FEE_VALUE);
Expand Down Expand Up @@ -143,7 +143,7 @@ mod receive {

Receiver::mock_receive(|middleware, origin, message| {
assert_eq!(middleware, Middleware(AxelarId::Evm(CHAIN_ID)));
assert_eq!(origin, DomainAddress::EVM(CHAIN_ID, SOURCE_ADDRESS.0));
assert_eq!(origin, DomainAddress::Evm(CHAIN_ID, SOURCE_ADDRESS.0));
assert_eq!(&message, MESSAGE);
Ok(())
});
Expand Down
15 changes: 6 additions & 9 deletions pallets/liquidity-pools-gateway/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ pub mod pallet {
T::AdminOrigin::ensure_origin(origin)?;

ensure!(
instance.domain() != Domain::Centrifuge,
instance.domain() != Domain::Local,
Error::<T>::DomainNotSupported
);

Expand Down Expand Up @@ -244,7 +244,7 @@ pub mod pallet {
) -> DispatchResult {
let GatewayOrigin::Domain(origin_address) = T::LocalEVMOrigin::ensure_origin(origin)?;

if let DomainAddress::Centrifuge(_) = origin_address {
if let DomainAddress::Local(_) = origin_address {
return Err(Error::<T>::InvalidMessageOrigin.into());
}

Expand All @@ -263,8 +263,8 @@ pub mod pallet {
) -> DispatchResult {
T::AdminOrigin::ensure_origin(origin)?;

ensure!(domain != Domain::Centrifuge, Error::<T>::DomainNotSupported);
DomainHookAddress::<T>::insert(domain.clone(), hook_address);
ensure!(domain != Domain::Local, Error::<T>::DomainNotSupported);
DomainHookAddress::<T>::insert(domain, hook_address);

Self::deposit_event(Event::DomainHookAddressSet {
domain,
Expand Down Expand Up @@ -377,12 +377,9 @@ pub mod pallet {
destination: Self::Destination,
message: Self::Message,
) -> DispatchResult {
ensure!(
destination != Domain::Centrifuge,
Error::<T>::DomainNotSupported
);
ensure!(destination != Domain::Local, Error::<T>::DomainNotSupported);

PackedMessage::<T>::mutate((&from, destination.clone()), |batch| match batch {
PackedMessage::<T>::mutate((&from, destination), |batch| match batch {
Some(batch) => batch.pack_with(message),
None => Self::queue_message(destination, message),
})
Expand Down
4 changes: 2 additions & 2 deletions pallets/liquidity-pools-gateway/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use frame_support::{derive_impl, weights::constants::RocksDbWeight};
use frame_system::EnsureRoot;
use parity_scale_codec::{Decode, Encode, MaxEncodedLen};
use scale_info::TypeInfo;
use sp_core::{crypto::AccountId32, H256};
use sp_core::crypto::AccountId32;
use sp_runtime::{traits::IdentityLookup, DispatchError, DispatchResult};

use crate::{pallet as pallet_liquidity_pools_gateway, EnsureLocal, GatewayMessage};
Expand Down Expand Up @@ -115,7 +115,7 @@ impl cfg_mocks::router_message::pallet::Config for Runtime {
}

frame_support::parameter_types! {
pub Sender: DomainAddress = DomainAddress::Centrifuge(AccountId32::from(H256::from_low_u64_be(1).to_fixed_bytes()).into());
pub Sender: DomainAddress = DomainAddress::Local([1; 32]);
pub const MaxIncomingMessageSize: u32 = 1024;
pub const LpAdminAccount: AccountId32 = LP_ADMIN_ACCOUNT;
}
Expand Down
7 changes: 1 addition & 6 deletions pallets/liquidity-pools-gateway/src/origin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ use cfg_types::domain_address::DomainAddress;
use frame_support::traits::EnsureOrigin;
use parity_scale_codec::{Decode, Encode, MaxEncodedLen};
use scale_info::TypeInfo;
#[cfg(feature = "runtime-benchmarks")]
use sp_core::H160;
use sp_runtime::RuntimeDebug;

#[derive(Clone, Eq, PartialEq, RuntimeDebug, Encode, Decode, MaxEncodedLen, TypeInfo)]
Expand All @@ -34,9 +32,6 @@ impl<O: Into<Result<GatewayOrigin, O>> + From<GatewayOrigin>> EnsureOrigin<O> fo

#[cfg(feature = "runtime-benchmarks")]
fn try_successful_origin() -> Result<O, ()> {
Ok(O::from(GatewayOrigin::Domain(DomainAddress::EVM(
1,
H160::from_low_u64_be(1).into(),
))))
unimplemented!()
}
}
Loading
Loading