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

Modify examples to work with latest substrate changes #440

Closed
wants to merge 5 commits into from

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Feb 7, 2022

This PR follows #439, and the CI should pull the latest substrate release.

lexnv added 5 commits February 7, 2022 19:58
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Comment on lines +41 to +42
for troubleshooting codegen as an alternative to `cargo expand`, and also provides the possibility of customizing the
generated code if the macro does not produce the desired API. e.g.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for troubleshooting codegen as an alternative to `cargo expand`, and also provides the possibility of customizing the
generated code if the macro does not produce the desired API. e.g.
for troubleshooting codegen as an alternative to `cargo expand`, and also provides the possibility to customize the
generated code if the macro does not produce the desired API. e.g.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Will open a new PR, examples are working on master

@@ -20,7 +20,7 @@
runtime_metadata_path = "examples/polkadot_metadata.scale",
// We can add (certain) custom derives to the generated types by providing
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious: do you happen to know what this "(certain)" means? Do we know which ones work and which don't?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't actually dug into this; I suspect we are limited by the derives on various types that are pointed at by the codegen types (eg sp_core::AccountId32 to use one example), so I think to find out we'd have to look through such types and find the common derives across them all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The subxt::Encode, subxt::Decode and Debug are already generated in polkadot.rs file.

Also std::Hash is problematic due to subxt::WrapperKeepOpaque<polkadot_runtime::Call> and PhantomDataSendSync. I guess I could address the Hash in a different PR.

@@ -70,7 +70,7 @@ async fn simple_transfer() -> Result<(), Box<dyn std::error::Error>> {
balance_transfer.find_first_event::<polkadot::balances::events::Transfer>()?;

if let Some(event) = transfer_event {
println!("Balance transfer success: value: {:?}", event.2);
println!("Balance transfer success: value: {:?}", event);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
println!("Balance transfer success: value: {:?}", event);
println!("Balance transfer success. Value: {:?}", event);

…but is it a "value" or an "event"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Mmm, it was a value but if we are printing out the entire event now by the looks of it, it's now just the Transfer event.

@@ -119,7 +119,7 @@ async fn simple_transfer_separate_events() -> Result<(), Box<dyn std::error::Err
let transfer_event =
events.find_first_event::<polkadot::balances::events::Transfer>()?;
if let Some(event) = transfer_event {
println!("Balance transfer success: value: {:?}", event.2);
println!("Balance transfer success: value: {:?}", event);
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@@ -20,7 +20,7 @@
/// Generate by:
///
/// - run `polkadot --dev --tmp` node locally
/// - `cargo run --release -p subxt-cli -- codegen | rustfmt --edition=2018 --emit=stdout > tests/integration/codegen/polkadot.rs`
/// - `cargo run --release -p subxt-cli -- codegen | rustfmt --edition=2018 --emit=stdout > subxt/tests/integration/codegen/polkadot.rs`
Copy link
Contributor

Choose a reason for hiding this comment

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

We're on edition 2021 I think. But also: do we really need that? I suspect just | rustfmt > subxt/…/… works too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree; I don't think either of the flags are needed actually (I always just rustfmt > outfile)

@@ -20,7 +20,7 @@
runtime_metadata_path = "examples/polkadot_metadata.scale",
// We can add (certain) custom derives to the generated types by providing
// a comma separated list to the below attribute. Most useful for adding `Clone`:
generated_type_derives = "Clone, Hash"
generated_type_derives = "Clone"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting; did Hash not work? (I think I'd have more than one if possible just so that people can see how to add multiple derives (and adding just one is fairly obvious from that :))

@dvdplm
Copy link
Contributor

dvdplm commented Feb 12, 2022

(I re-ran the CI jobs – looks like crates.io was unreachable)

@jsdw
Copy link
Collaborator

jsdw commented Feb 14, 2022

Sorry about some of the file conflicts; I had a mess with the examples in the event subscription stuff!

There are a couple of nice doc fixes in here; @lexnv would you like to push to get this merged or would you prefer to close it in favour of a new PR?

@lexnv lexnv mentioned this pull request Feb 14, 2022
@lexnv
Copy link
Collaborator Author

lexnv commented Feb 14, 2022

Closed in favor of #449.

@lexnv lexnv closed this Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants