Refactor: Unify DomainAddress and account conversions (r2) #1969

lemunozm · 2024-08-15T23:38:21Z

This PRs overwrites #1967

Description

Represents DomainAddress as:

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

With some utility methods as:

fn account() -> AccountId32;
fn h160() -> H160;
fn bytes() -> [u8; 32];

Main changes

Removed account conversions through Convert traits. Now DomainAddress knows how to convert accounts.
Unify [u8; 20] to [u8;32] conversions. See this slack thread
Renamed EVM to Evm (following rust guidelines)
(only in IT). Removed From/Into traits from Keyring to H160 and [u8;20] and use instead an explicit method called in_eth() (open to name proposals). This method represents the account in the Ethereum network. Note that this differs from Keyring::Alice.into()[0..20], which is used in other places where an Ethereum account is stored as an AccountId and needs to be recovered. Because this is highly confusing, I've removed the into/from methods to be more explicit about when the first case happens.

lemunozm · 2024-08-15T23:43:02Z

@wischli @cdamian I tried my best from your inputs in the other PR. I would really love to have types who knows about conversions but it's quite tedious for Monday (we also will need implement them with a lot of traits, etc).

I think this solution leaves things easier than before, but of course, it still can be improved with more sophisticated solutions.

Let me know if you agree more with the names than the before PR.

lemunozm · 2024-08-15T23:45:33Z

pallets/liquidity-pools/src/lib.rs

@@ -131,7 +132,7 @@ pub mod pallet {
 	pub struct Pallet<T>(_);

 	#[pallet::config]
-	pub trait Config: frame_system::Config {
+	pub trait Config: frame_system::Config<AccountId = AccountId32> {


Everything works fine in IT except for a compilation error with the fudge environment. I bet you, guys, to give it a try... I had never experienced it before.

You, that error is weird:

error[E0284]: type annotations needed: cannot satisfy `<T as frame_system::Config>::AccountId == sp_runtime::AccountId32` --> runtime/integration-tests/src/envs/fudge_env.rs:130:9 | 130 | impl<T: Runtime + FudgeSupport> FudgeEnv<T> { | ^^^^^^^ cannot satisfy `<T as frame_system::Config>::AccountId == sp_runtime::AccountId32` | note: required by a bound in `config::Runtime` --> runtime/integration-tests/src/config.rs:61:3 | 56 | pub trait Runtime: | ------- required by a bound in this trait ... 61 | AccountId = AccountId, | ^^^^^^^^^^^^^^^^^^^^^ required by this bound in `Runtime`

And I assume you restricted AccountId to AccountId32 so that you can easily call domain_address.account().

But I think domain_address.account() should return AccountId instead of AccountId32.

And I assume you restricted AccountId to AccountId32 so that you can easily call domain_address.account()

Yes, anyways, we were using From<[u8; 32]> and Into<[u8; 32]> as a restriction in that pallet.

But I think domain_address.account() should return AccountId instead of AccountId32.

AccountId is only a type alias for AccountId32, which should not affect it. Ideally, AccountId is (should be) a runtime definition. I used the real type AccountId32, just to be more close to the baseline

fn foo<T: Runtime + FudgeSupport>(f: ApiRefOf<T>) {} // compiles

fn bar<T: Runtime + FudgeSupport, F: FnOnce(ApiRefOf<T>)>(f: F) {} //no compiles

I'm reaching the boundaries of the rust compiler? 😲

Finally, I fixed getting the type in another, more simpler, way

cdamian · 2024-08-16T07:47:59Z

libs/types/src/ids.rs

-	const TYPE_ID: [u8; 4] = *b"domn";
-}
+pub const DOMAIN_ID: [u8; 4] = *b"dadr";
+pub const DOMAIN_ADDRESS_ID: [u8; 4] = *b"domn";


The values of these 2 constants should be swapped?

Oh man, blessing eyes yours 🙌🏻

lemunozm · 2024-08-16T15:38:51Z

runtime/integration-tests/src/cases/example.rs

@@ -68,7 +68,7 @@ fn transfer_balance<T: Runtime>() {
 }

 // Identical to `transfer_balance()` test but using fudge.
-#[test_runtimes([development, altair, centrifuge])]
+#[test_runtimes([development, altair, centrifuge], ignore = "uncomment to run the example")]


Saving some extra CI time with those tests ignored

lemunozm · 2024-08-16T15:50:21Z

runtime/integration-tests/src/cases/liquidity_pools_gateway_queue.rs

 use sp_runtime::{traits::One, BoundedVec};

 use crate::{
-	cases::liquidity_pools::{DEFAULT_DOMAIN_ADDRESS_MOONBEAM, DEFAULT_ROUTER_ID},


I wanted to enforce the rule that things should not be imported from one cases file to another.

My rationale for doing this:

If cases::liquidity_pools needs to be refactorized due to some new feature, we're probably breaking also the queue tests (when we shouldn't).

If a reader wants to understand these queue tests, we force them to also look into liquidity_pools tests to fully understand when both are theoretically unrelated.

Of course, if the common code grows, we should factorize this into some well-through enough-generic utility module. Then, both cases can point to those logic without carrying mutual information.

cc @cdamian @wischli

Thanks for bringin this up. I definitely agree!

lemunozm · 2024-08-16T15:57:47Z

runtime/integration-tests/src/envs/fudge_env.rs

@@ -46,8 +46,6 @@ impl<T: Runtime + FudgeSupport> Env<T> for FudgeEnv<T> {
 		parachain_storage: Storage,
 		sibling_storage: Storage,
 	) -> Self {
-		crate::utils::logs::init_logs();


I wanted to remove this because I really think it does not offer us anything to have it by default. Most errors are pallet_timestampt errors or migrations errors that do not affect the test case, offuscating the CI results and local tests.

99% of the test case failures are not something "found" in the logs. If the test requires more debugging, we can always enable it again with...

lemunozm · 2024-08-16T15:58:05Z

runtime/integration-tests/src/README.md

+If you want to add logs to your use case for debugging purposes, simply add
+
+```rust
+crate::utils::logs::init_logs();


...just adding this line

Thanks for reverting the default logger which has been super noisy.

wischli

Once again, thanks for cleaning up this mess! I'd be glad to merge this, because its needed for #1971.

As for the naming of fn bytes(), I am not 100% on board but that can be changed at any time, if at all, because it has zero impact.

wischli · 2024-08-16T16:33:04Z

runtime/integration-tests/src/README.md

+If you want to add logs to your use case for debugging purposes, simply add
+
+```rust
+crate::utils::logs::init_logs();


Thanks for reverting the default logger which has been super noisy.

wischli · 2024-08-16T16:34:01Z

runtime/integration-tests/src/cases/liquidity_pools_gateway_queue.rs

 use sp_runtime::{traits::One, BoundedVec};

 use crate::{
-	cases::liquidity_pools::{DEFAULT_DOMAIN_ADDRESS_MOONBEAM, DEFAULT_ROUTER_ID},


Thanks for bringin this up. I definitely agree!

lemunozm · 2024-08-16T16:50:19Z

As for the naming of fn bytes(), I am not 100% on board

I wanted to not use address because it's quite generic, could be not just an array, and for that method we're specifying that we want the account as an array of bytes.

Rust names *_bytes() to actions that return an array, but I am open to other names, I can do it in a next PR if needed

lemunozm · 2024-08-16T18:18:39Z

I'm very happy to have this PR before the audit! 🎉

lemunozm added 11 commits August 15, 2024 12:51

runtime-common compiling

9cac0c1

runtimes compiling

a76eba5

add IT required changes

634d2ee

add id_eth

6db6ca9

rename id_eth to in_eth

ca9efa0

fix lp UTs, still pending the inbounds

95d9148

finish lp UTs

81ee4a5

fix IT last tests

6bbf27d

fix clippy and benchmarking

5825728

fix issues after rebasing

84c03a8

fix IT

764e4b2

lemunozm added the I6-refactoring Code needs refactoring. label Aug 15, 2024

lemunozm requested review from cdamian and wischli August 15, 2024 23:38

lemunozm self-assigned this Aug 15, 2024

lemunozm mentioned this pull request Aug 15, 2024

Refactor: Unify DomainAddress and account conversions #1967

Closed

lemunozm commented Aug 15, 2024

View reviewed changes

base refactor

30b3859

lemunozm force-pushed the ref/domain-address-r2 branch from 99c00ae to 30b3859 Compare August 15, 2024 23:46

cdamian reviewed Aug 16, 2024

View reviewed changes

lemunozm added 3 commits August 16, 2024 15:44

fix ids

650284c

fix weird compilation issue

dbb14ff

fix clippy

e9499d0

lemunozm marked this pull request as ready for review August 16, 2024 14:52

Merge remote-tracking branch 'origin/main' into ref/domain-address-r2

e0d8401

lemunozm commented Aug 16, 2024

View reviewed changes

minor change

3c9e5b4

wischli approved these changes Aug 16, 2024

View reviewed changes

lemunozm enabled auto-merge (squash) August 16, 2024 16:50

cdamian approved these changes Aug 16, 2024

View reviewed changes

lemunozm merged commit eb7e14b into main Aug 16, 2024
9 of 11 checks passed

lemunozm deleted the ref/domain-address-r2 branch August 16, 2024 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Unify DomainAddress and account conversions (r2) #1969

Refactor: Unify DomainAddress and account conversions (r2) #1969

lemunozm commented Aug 15, 2024 •

edited

Loading

lemunozm commented Aug 15, 2024 •

edited

Loading

lemunozm Aug 15, 2024

cdamian Aug 16, 2024

cdamian Aug 16, 2024

cdamian Aug 16, 2024

lemunozm Aug 16, 2024

lemunozm Aug 16, 2024

lemunozm Aug 16, 2024

cdamian Aug 16, 2024 •

edited

Loading

lemunozm Aug 16, 2024

lemunozm Aug 16, 2024

lemunozm Aug 16, 2024

wischli Aug 16, 2024

lemunozm Aug 16, 2024

lemunozm Aug 16, 2024

wischli Aug 16, 2024

wischli left a comment

wischli Aug 16, 2024

wischli Aug 16, 2024

lemunozm commented Aug 16, 2024 •

edited

Loading

lemunozm commented Aug 16, 2024

Refactor: Unify DomainAddress and account conversions (r2) #1969

Refactor: Unify DomainAddress and account conversions (r2) #1969

Conversation

lemunozm commented Aug 15, 2024 • edited Loading

Description

Main changes

lemunozm commented Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdamian Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Aug 16, 2024 • edited Loading

lemunozm commented Aug 16, 2024

lemunozm commented Aug 15, 2024 •

edited

Loading

lemunozm commented Aug 15, 2024 •

edited

Loading

cdamian Aug 16, 2024 •

edited

Loading

lemunozm commented Aug 16, 2024 •

edited

Loading