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.

Sign up for GitHub

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

Jump to bottom

Fix compact event field decoding #384

Merged
merged 8 commits into from
Jan 7, 2022
Merged

Fix compact event field decoding #384

merged 8 commits into from
Jan 7, 2022

Conversation

ascjones
Copy link
Contributor

@ascjones ascjones commented Jan 5, 2022 •
edited
Loading

Events with compact fields were failing to be decoded. Adds unit tests for event decoding.

Fixes #383 (comment)

@ascjones ascjones mentioned this pull request Jan 5, 2022
Closed
@paulormart
Copy link
Contributor

Thanks for this @ascjones.

Not particular related to the fix of this PR, but it seems that the latest updates of jsonrpsee brakes the fetch_remote example which is needed from my side to validate this PR. On my fork i'm still using a really old version of jsonrpsee

Are you able to run this cargo watch -x 'run --example fetch_remote'?

When setting .set_url("ws://rpc.polkadot.io:80") as suggested from the error message i still get Error: Rpc(Transport(Invalid URL: URL scheme not supported, expects 'ws'))

@niklasad1
Copy link
Member

niklasad1 commented Jan 5, 2022 •
edited
Loading

@paulormart

please open an issue in jsonrpsee but the issue is that you got a HTTP redirection there... to a HTTP endpoint

target: Target { sockaddrs: [104.26.8.125:80, 104.26.9.125:80, 172.67.71.26:80, [2606:4700:20::681a:97d]:80, [2606:4700:20::ac43:471a]:80, [2606:4700:20::681a:87d]:80], host: "rpc.polkadot.io", host_header: "rpc.polkadot.io:80", _mode: Plain, path_and_query: "/" }
2022-01-05T15:32:34.132404Z DEBUG jsonrpsee_client_transport::ws: Connecting to target: Target { sockaddrs: [104.26.8.125:80, 104.26.9.125:80, 172.67.71.26:80, [2606:4700:20::681a:97d]:80, [2606:4700:20::ac43:471a]:80, [2606:4700:20::681a:87d]:80], host: "rpc.polkadot.io", host_header: "rpc.polkadot.io:80", _mode: Plain, path_and_query: "/" }
2022-01-05T15:32:34.266608Z DEBUG jsonrpsee_client_transport::ws: Redirection: status_code: 301, location: https://rpc.polkadot.io/
thread 'main' panicked at 'Some("https")', client/transport/src/ws/mod.rs:440:17

Thus, meanwhile use wss://rpc.polkadot.io:443 if that's alright for you.

@paulormart paulormart mentioned this pull request Jan 5, 2022
Closed
@paulormart
Copy link
Contributor

Hi @ascjones this PR fixes #383

Base automatically changed from aj-config to master January 6, 2022 09:39
@ascjones ascjones force-pushed the aj/fix-compact-event-field-decoding branch from 62c0338 to 10c3d51 Compare January 6, 2022 09:56
@ascjones ascjones marked this pull request as ready for review January 6, 2022 09:56
@ascjones ascjones requested a review from jsdw January 6, 2022 09:57
jsdw
jsdw reviewed Jan 6, 2022
View reviewed changes
src/events.rs Show resolved Hide resolved
@jsdw jsdw requested a review from a team January 7, 2022 11:35
niklasad1
niklasad1 reviewed Jan 7, 2022
View reviewed changes
src/events.rs
meta_type,
TypeInfo,
};
use std::convert::TryFrom;
Copy link
Member

Choose a reason for hiding this comment

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

we are on edition 2021 right?

Suggested change
use std::convert::TryFrom;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we are, sorry I missed this comment before merging 😬

niklasad1
niklasad1 approved these changes Jan 7, 2022
View reviewed changes
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM, neat

niklasad1
niklasad1 reviewed Jan 7, 2022
View reviewed changes
src/lib.rs
@@ -166,7 +166,7 @@ impl codec::Encode for Encoded {
}

/// A phase of a block's execution.
#[derive(Clone, Debug, Eq, PartialEq, Decode)]
#[derive(Clone, Debug, Eq, PartialEq, Decode, Encode)]
Copy link
Member

Choose a reason for hiding this comment

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

was this required compact encoding/decoding to work or a bonus? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for the tests in order to encode some EventRecords

@ascjones ascjones merged commit e8cbe46 into master Jan 7, 2022
@ascjones ascjones deleted the aj/fix-compact-event-field-decoding branch January 7, 2022 14:20
@ascjones ascjones mentioned this pull request Jan 7, 2022
Merged
0623forbidden pushed a commit to DEIPworld/substrate-subxt that referenced this pull request Feb 15, 2022
@ascjones @0623forbidden
Fix compact event field decoding (paritytech#384)
12a5ec0 
* Add basic event decoding test

* Failing compact event field test

* Fmt

* Fix compact event field decoding

* Remove println

* Add test for compact wrapper struct

* Revert "Add test for compact wrapper struct"

This reverts commit 4e8332d.

* Split compact tests and add multiple events test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niklasad1 niklasad1 niklasad1 approved these changes

@jsdw jsdw jsdw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Events at Initialization phase are silently being ignored if ImOnline::SomeOffline event is present
4 participants
@ascjones @paulormart @niklasad1 @jsdw