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

Make UncheckedExtrinsicV4 generic #418

Merged
merged 6 commits into from
Jan 11, 2023
Merged

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Jan 5, 2023

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.

  • introduced SignExtrinsic trait to simplify the Address, AccountId and Signature allocation as much as possible.
  • The default implementation ExtrinsicSigner should be usable for most use cases, except for when no Runtime is used. In that case one still has to option to implement the trait oneselves.
  • UncheckedExtrinsic now implements a new_unsigned function.

Drawback: Signature type needs to be defined explicitly. However, #406 may help with that.

closes #360

@haerdib haerdib self-assigned this Jan 5, 2023
@haerdib haerdib added F7-enhancement Enhances an already existing functionality E2-breaksapi labels Jan 5, 2023
@haerdib haerdib changed the title Allow generic Unchecked Extrinsic and compose macros Make UncheckedExtrinsic generic Jan 5, 2023
@haerdib haerdib marked this pull request as ready for review January 5, 2023 12:02
@haerdib haerdib changed the title Make UncheckedExtrinsic generic Make UncheckedExtrinsicV4 generic Jan 5, 2023
@haerdib haerdib marked this pull request as draft January 5, 2023 12:09
@haerdib haerdib marked this pull request as ready for review January 5, 2023 12:15
@haerdib haerdib force-pushed the bh/generic-unchecked-extrinsic branch 3 times, most recently from 7090df1 to da78380 Compare January 9, 2023 12:33
Copy link
Contributor

@Niederb Niederb left a 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 =
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@haerdib haerdib requested a review from Niederb January 10, 2023 13:54
Copy link
Collaborator

@clangenb clangenb left a 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. :-)

Comment on lines 47 to 50
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);
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

haerdib and others added 5 commits January 11, 2023 08:20
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>
@haerdib haerdib force-pushed the bh/generic-unchecked-extrinsic branch from 897f5be to 438fe00 Compare January 11, 2023 07:28
@haerdib haerdib merged commit 027ed59 into master Jan 11, 2023
@haerdib haerdib deleted the bh/generic-unchecked-extrinsic branch January 11, 2023 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E2-breaksapi F7-enhancement Enhances an already existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make UncheckedExtrinsicV4 generic over AccountId
3 participants