-
Notifications
You must be signed in to change notification settings - Fork 84
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 (r2) #1969
Conversation
@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. |
@@ -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> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I assume you restricted AccountId
to AccountId32
so that you can easily call domain_address.account()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think domain_address.account()
should return AccountId
instead of AccountId32
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? 😲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally, I fixed getting the type in another, more simpler, way
99c00ae
to
30b3859
Compare
libs/types/src/ids.rs
Outdated
const TYPE_ID: [u8; 4] = *b"domn"; | ||
} | ||
pub const DOMAIN_ID: [u8; 4] = *b"dadr"; | ||
pub const DOMAIN_ADDRESS_ID: [u8; 4] = *b"domn"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values of these 2 constants should be swapped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh man, blessing eyes yours 🙌🏻
@@ -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")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saving some extra CI time with those tests ignored
use sp_runtime::{traits::One, BoundedVec}; | ||
|
||
use crate::{ | ||
cases::liquidity_pools::{DEFAULT_DOMAIN_ADDRESS_MOONBEAM, DEFAULT_ROUTER_ID}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 intoliquidity_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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringin this up. I definitely agree!
@@ -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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
If you want to add logs to your use case for debugging purposes, simply add | ||
|
||
```rust | ||
crate::utils::logs::init_logs(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...just adding this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reverting the default logger which has been super noisy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
If you want to add logs to your use case for debugging purposes, simply add | ||
|
||
```rust | ||
crate::utils::logs::init_logs(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reverting the default logger which has been super noisy.
use sp_runtime::{traits::One, BoundedVec}; | ||
|
||
use crate::{ | ||
cases::liquidity_pools::{DEFAULT_DOMAIN_ADDRESS_MOONBEAM, DEFAULT_ROUTER_ID}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringin this up. I definitely agree!
I wanted to not use Rust names |
I'm very happy to have this PR before the audit! 🎉 |
This PRs overwrites #1967
Description
Represents
DomainAddress
as:With some utility methods as:
Main changes
Convert
traits. NowDomainAddress
knows how to convert accounts.[u8; 20]
to[u8;32]
conversions. See this slack threadEVM
toEvm
(following rust guidelines)From/Into
traits fromKeyring
toH160
and[u8;20]
and use instead an explicit method calledin_eth()
(open to name proposals). This method represents the account in the Ethereum network. Note that this differs fromKeyring::Alice.into()[0..20]
, which is used in other places where an Ethereum account is stored as anAccountId
and needs to be recovered. Because this is highly confusing, I've removed theinto/from
methods to be more explicit about when the first case happens.