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

Clean up trait bounds #541

Merged
merged 3 commits into from
May 4, 2023
Merged

Clean up trait bounds #541

merged 3 commits into from
May 4, 2023

Conversation

echevrier
Copy link
Contributor

@echevrier echevrier commented Apr 18, 2023

Fixes: #406
The different types that are important to communicate with a specific node can be configured.

All examples use the substrate kitchensink runtime: the config is a substrate config with the kitchensink runtime block type

Issue #504 is obsolete. The ExtrinsicSigner does no more depend on Runtime input but from the configuration. It is cloneable. The StaticExtrinsicSigner is still available but instead of StaticExtrinsicSigner, we can use ExtrinsicSigner::<SubstrateConfig, sr25519::Pair>.

@echevrier echevrier force-pushed the ec/clean_up branch 2 times, most recently from 4d228f6 to fa214de Compare April 19, 2023 07:53
examples/examples/benchmark_bulk_xt.rs Outdated Show resolved Hide resolved
examples/examples/benchmark_bulk_xt.rs Outdated Show resolved Hide resolved
primitives/src/config/substrate_config.rs Outdated Show resolved Hide resolved
primitives/src/config/substrate_config.rs Outdated Show resolved Hide resolved
primitives/src/config/mod.rs Outdated Show resolved Hide resolved
src/api/api_client.rs Outdated Show resolved Hide resolved
type Index = u32;
type Hash = H256;
type AccountId = AccountId32;
type Address = MultiAddress<Self::AccountId, u32>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AccountIndex and BlockNumber types are now a little hidden. I don't really see the benefit of not defiing these types.
E.g. add the types

type AccountIndex = u32;
type BlockNumber = u32;

And then assigned Header and Address with Self::BlockNumber / AccountIndex ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with the AccountIndex. Regarding the BlockNumber I guess it's your choice. I think it makes it more complex than it helps, subxt thinks otherwise. Up to you :)

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 introduced the BlockNumber, to simplify our code. As this is the goal.

src/api/rpc_api/chain.rs Outdated Show resolved Hide resolved
src/api/rpc_api/events.rs Outdated Show resolved Hide resolved
testing/examples/author_tests.rs Outdated Show resolved Hide resolved
@haerdib
Copy link
Contributor

haerdib commented Apr 19, 2023

Sorry, I was so immersed in the code, that I totally forgot to mention: Thanks a lot for this PR! I think it's a huge improvement! 🥳

@echevrier echevrier linked an issue Apr 19, 2023 that may be closed by this pull request
2 tasks
@echevrier echevrier requested a review from haerdib April 26, 2023 08:20
@echevrier echevrier linked an issue Apr 26, 2023 that may be closed by this pull request
Copy link
Contributor

@haerdib haerdib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice ! I really like it 🥳 Only small change requests, other than that I approve 👍

examples/examples/get_storage.rs Outdated Show resolved Hide resolved
primitives/src/extrinsics/mod.rs Outdated Show resolved Hide resolved
primitives/src/extrinsics/signer.rs Outdated Show resolved Hide resolved
primitives/src/extrinsics/signer.rs Show resolved Hide resolved
primitives/src/extrinsics/signer.rs Outdated Show resolved Hide resolved
testing/examples/pallet_balances_tests.rs Outdated Show resolved Hide resolved
testing/examples/pallet_balances_tests.rs Outdated Show resolved Hide resolved
testing/examples/pallet_transaction_payment_tests.rs Outdated Show resolved Hide resolved
testing/examples/pallet_transaction_payment_tests.rs Outdated Show resolved Hide resolved
testing/examples/pallet_transaction_payment_tests.rs Outdated Show resolved Hide resolved
@haerdib
Copy link
Contributor

haerdib commented Apr 28, 2023

One thing: Can the Pallet Configs be removed now? E.g FrameSystemConfig and such.

@haerdib haerdib mentioned this pull request Apr 28, 2023
1 task
@echevrier
Copy link
Contributor Author

One thing: Can the Pallet Configs be removed now? E.g FrameSystemConfig and such.

FrameSystemConfig is still used by

let xt_hash1 = <Runtime as FrameSystemConfig>::Hashing::hash_of(&xt1.0);

echevrier added 2 commits May 2, 2023 13:42
- Remove ExtrinsicParams from API. Add them to Config.
- Remove Runtime from API:FrameSystemConfig,BalancesConfig,ContractsConfig
- Add AccountData in primitives type
- Add Block to API, as this can't be easily configured (requires the Call from the runtime)
- Add config trait similar to subxt and a specific config for substrate node
- Get the block number from Header

Remove Signer
Add StakingBalance in config
Add a polkadot config
Example and test compile and run succesffully

Move Blocktype from API into config

Clean examples: all use kitchensing runtime

Clean code

move config into own folder

Clean after merge

Clippy

Remove unnecessary bounds

Header::Number must implement copy

Changes from review

Changes from review

Remove Signer from ExtrinsicSigner

Changes from Review:
- add type BlockNumber
- add trait file and move hasher and Header traits there

Changes from Review: make config generic over Call and use a own SubstrateBlock type
- Define a specific SubstrateExtrinsic type
- Implement Extrinsic,Serialize and Deserialize for UncheckedExtrinsicV4
- add unit tests

Use of OpaqueExtrinsic: no need of Call input in Config.
Add own OpaqueExtrinsic for no-std serialization
@echevrier
Copy link
Contributor Author

One thing: Can the Pallet Configs be removed now? E.g FrameSystemConfig and such.

FrameSystemConfig is still used by

let xt_hash1 = <Runtime as FrameSystemConfig>::Hashing::hash_of(&xt1.0);

I removed the dependencies and deleted the pallet configs

Copy link
Contributor

@haerdib haerdib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Very nice. Only one comment left :)

fn hash(&self) -> <Self::Hasher as Hasher>::Output {
<<Self::Header as Header>::Hasher>::hash_of(self.header())
}
//Self::Hash {
Copy link
Contributor

@haerdib haerdib May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still needed?

{
type ExtrinsicAddress<T> =
<<T as Config>::ExtrinsicSigner as SignExtrinsic<<T as Config>::AccountId>>::ExtrinsicAddress;
type Signature<T> =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's a pity. But yeah, makes sense. Leave as is then

@echevrier echevrier requested a review from haerdib May 3, 2023 06:40
Copy link
Contributor

@haerdib haerdib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks a lot 🥳

@echevrier echevrier merged commit caf8d84 into master May 4, 2023
@echevrier echevrier deleted the ec/clean_up branch May 4, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExtrinsicSigner struct does not allow clone Allow Opaque types Clean up trait bounds
2 participants