centrifuge / centrifuge-chain

Centrifuge Chain: the layer-1 blockchain for real-world assets, built on Substrate.
https://centrifuge.io
GNU Lesser General Public License v3.0
182 stars 78 forks source link

Refactor: Unify DomainAddress and account conversions #1967

Closed lemunozm closed 4 weeks ago

lemunozm commented 1 month ago

Description

lemunozm commented 1 month ago

Current implementation is done as:

pub enum DomainAddress {
    Local([u8; 32]),        
    Evm(EVMChainId, [u8; 20]),
}

But I'm thinking of changing this into:

pub enum DomainAddress {
    Centrifuge(AccountId32),        
    Evm(EVMChainId, H160),
}

I do not like that we expose the AccountId32 type (which, theoretically, is set at runtime), but we would gain much more type safety. It's a pain to deal with arrays. Maybe it makes sense to expose an AccountId32 because speaking about a "Centrifuge domain" inherently carries some "domain information"

EDIT: after playing with the second approach, I feel like it's harder to deal with in a lot of scenarios, so I will keep the first model.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 60.34483% with 46 lines in your changes missing coverage. Please review.

Project coverage is 47.76%. Comparing base (e8f1efd) to head (764e4b2).

Files Patch % Lines
pallets/liquidity-pools/src/hooks.rs 0.00% 10 Missing :warning:
libs/types/src/domain_address.rs 76.66% 7 Missing :warning:
node/src/chain_spec.rs 0.00% 6 Missing :warning:
pallets/liquidity-pools-gateway/src/lib.rs 0.00% 6 Missing :warning:
pallets/liquidity-pools/src/lib.rs 85.18% 4 Missing :warning:
runtime/common/src/gateway.rs 0.00% 4 Missing :warning:
runtime/common/src/routing.rs 0.00% 3 Missing :warning:
runtime/common/src/account_conversion.rs 0.00% 2 Missing :warning:
pallets/axelar-router/src/lib.rs 66.66% 1 Missing :warning:
runtime/altair/src/lib.rs 0.00% 1 Missing :warning:
... and 2 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1967 +/- ## ======================================= Coverage 47.76% 47.76% ======================================= Files 183 183 Lines 12929 12887 -42 ======================================= - Hits 6176 6156 -20 + Misses 6753 6731 -22 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lemunozm commented 4 weeks ago

This PR wants to change 6e8f1a29dff0d7cf5ff74285cfffadae8a8b303f which is origin/main to 4301885b9a3b8ec36f3bda4b789daa5b115c006a

Good point! It was during the rebase...

lemunozm commented 4 weeks ago

Given the change from Local to Centrifuge, I should also change the as_local() and from_local(), I think both could be: as_account() (or just account()) and from_account().

Do you agree or do you have some preference? @wischli @cdamian

wischli commented 4 weeks ago

Given the change from Local to Centrifuge, I should also change the as_local() and from_local(), I think both could be: as_account() (or just account()) and from_account().

Do you agree or do you have some preference? @wischli @cdamian

Sounds good to me! {as, from}_centrifuge certainly sounds wrong. If we want to be more explicit, we could also use ..account32.. or ..account_id...

lemunozm commented 4 weeks ago

Well, I need to think a bit more because what's returned is the 32-bit array, so the account name may not make sense in some places where the array is expected (as when we construct a message with an eth address expanded to 32 bytes). Such a dilemma!

lemunozm commented 4 weeks ago

Closed in favor of: https://github.com/centrifuge/centrifuge-chain/pull/1969