Skip to content

decode option event arg #158

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
Merged
ascjones merged 1 commit into from
Sep 7, 2020
Merged

decode option event arg #158

ascjones merged 1 commit into from
Sep 7, 2020

Conversation

@gregdhill
Copy link
Contributor

@gregdhill gregdhill commented Sep 3, 2020 •
edited
Loading

Signed-off-by: Gregory Hill [email protected]

A lot of code shift here is due to cargo fmt, I can revert this if needs be. The main change here is to enable Option decoding, required for PolkaBTC.

EDIT: cargo +nightly fmt fixed it, thanks @dvc94ch

@cla-bot-2020
Copy link

cla-bot-2020 bot commented Sep 3, 2020

@gregdhill it looks like you have not signed our contributor license aggreement yet. Please visit this link to sign our agreement. This pull request cannot be merged until the agrement is signed.

@cla-bot-2020
Copy link

cla-bot-2020 bot commented Sep 3, 2020

@gregdhill, Your signature has been received.

@dvc94ch
Copy link
Contributor

dvc94ch commented Sep 3, 2020

Can you run cargo +nightly fmt and push?

@dvc94ch
Copy link
Contributor

dvc94ch commented Sep 3, 2020

Looks reasonable to me, thanks! @ascjones is the repo master

ascjones
ascjones suggested changes Sep 3, 2020
View reviewed changes
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

Thanks for this - but from what I can tell it might not work because the Option is not decoded correctly, i.e. it should first read the leading byte. If you've tested it and it works happy to be proved wrong.

@gregdhill
Copy link
Contributor Author

gregdhill commented Sep 7, 2020

Thanks for the feedback @ascjones, admittedly I had not dug deep enough into the serialization logic so I appreciate the guidance. I've updated the PR so please let me know what you think!

ascjones
ascjones reviewed Sep 7, 2020
View reviewed changes
@gregdhill
decode option event arg
3de2e02 
Signed-off-by: Gregory Hill <[email protected]>
ascjones
ascjones approved these changes Sep 7, 2020
View reviewed changes
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

@ascjones ascjones merged commit e85d01e into paritytech:master Sep 7, 2020
@gregdhill gregdhill deleted the option branch September 7, 2020 14:43
dvdplm added a commit that referenced this pull request Sep 15, 2020
@dvdplm
Merge branch 'master' into staking
773d43e 
* master:
  decode option event arg (#158)
  Release v0.12.0 (#157)
  Only return an error if the extrinsic failed. (#156)
  Update to rc6. (#155)
  Different assert. (#153)
  Add a method to fetch an unhashed key, close #100 (#152)
  Fix port number. (#151)
@dvdplm dvdplm mentioned this pull request Sep 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ascjones ascjones ascjones approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gregdhill @dvc94ch @ascjones