-
Notifications
You must be signed in to change notification settings - Fork 125
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
Make UncheckedExtrinsicV4 generic #418
Conversation
7090df1
to
da78380
Compare
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.
LGTM. I like the new SignExtrinsic
and I think it makes sense to have a UncheckedExtrinsicV4
that is similar to substrate
|
||
let recipient = AccountKeyring::Bob.to_account_id(); | ||
let recipient: <MyExtrinsicSigner as SignExtrinsic<AccountId>>::ExtrinsicAddress = |
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.
As discussed: I'm a bit confused by this syntax. I would expect that the as ...
part is not neccessary
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.
Sadly it is:
error[E0223]: ambiguous associated type
--> examples/examples/benchmark_bulk_xt.rs:43:17
|
43 | let recipient: MyExtrinsicSigner::ExtrinsicAddress = AccountKeyring::Bob.to_account_id().into();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use fully-qualified syntax: `<ExtrinsicSigner<sp_core::sr25519::Pair, MultiSignature, kitchensink_runtime::Runtime> as Trait>::ExtrinsicAddress`
But let me define a better type for that.
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'm guessing it's because of the trait type AccountId
which needs to be defined.
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.
Only briefly touched it. It looks good in general!
However, I wonder if we are trading off too much understandability and simplicity for hardly used generics.
Forget, what I said, I think the overall usability does still increase a lot with this change. :-)
testing/examples/author_tests.rs
Outdated
let bob: ExtrinsicAddressOf<ExtrinsicSigner> = AccountKeyring::Bob.to_account_id().into(); | ||
|
||
// Submit extrinisc. | ||
let xt0 = api.balance_transfer(bob.clone(), 1000); | ||
let xt0 = api.balance_transfer(bob.clone().into(), 1000); |
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 have double into
here. You can remove the annoying type annotation if you just remove the first one.
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.
Good catch! I removed the second one, though, because the .into() would need to be repeated up to 6 times afterwards.
wohoo fix most finally compiling make clippy happy remove unused comment update comment Fix documentation for running an example (#417) Also some minor typos clean up imports of macors Remove thiserror and use more specific Error names (#419) * small result clean up * lets not overcomplicate * remove thiserror * remove unused errors fix build Bump tokio from 1.23.0 to 1.23.1 (#427) Bumps [tokio](https://github.com/tokio-rs/tokio) from 1.23.0 to 1.23.1. - [Release notes](https://github.com/tokio-rs/tokio/releases) - [Commits](tokio-rs/tokio@tokio-1.23.0...tokio-1.23.1) --- updated-dependencies: - dependency-name: tokio dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
897f5be
to
438fe00
Compare
UncheckedExtrinsic
is now generic over the AccountId, Signer and Address type. All types of substrate nodes should now be supported, no matter which Account types are used.SignExtrinsic
trait to simplify theAddress
,AccountId
andSignature
allocation as much as possible.ExtrinsicSigner
should be usable for most use cases, except for when noRuntime
is used. In that case one still has to option to implement the trait oneselves.UncheckedExtrinsic
now implements anew_unsigned
function.Drawback:
Signature
type needs to be defined explicitly. However, #406 may help with that.closes #360