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

Add parsing for signed payload #63

Closed

Conversation

elliot-u410
Copy link

@elliot-u410 elliot-u410 commented Oct 28, 2021

Looking for initial thoughts.

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Oct 28, 2021

User @Noch3, please sign the CLA here.

@elliot-u410
Copy link
Author

I'm working with my employer to sign the CLA. It might take a while.

let address = <MultiAddress<AccountId32, u32>>::decode(data)?;
let signature = MultiSignature::decode(data)?;
let extensions = metadata
fn decode_call_data<'data, 'meta>(
Copy link
Author

Choose a reason for hiding this comment

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

I'm wondering if all of these sub-parsers should also be pub since they could be quite useful in non-standard scenarios.

Copy link
Collaborator

@jsdw jsdw Nov 1, 2021

Choose a reason for hiding this comment

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

I've been hesitant to make too many things pub at the moment becaue this V14 crate is still fairly heavily WIP, so I've been learning towards avoiding exposing too many things from the start and making it a little harder to keep hacking on as we go forwards without breaking APIs.

w.r.t the parsing, I'd sortof have assumed that the top level parser + scale_info would be enough (given some type and some bytes, you'll get a decoded Value back), but I'd be interested to hear about any cases where the sub parsers were useful on their own!

Copy link
Author

Choose a reason for hiding this comment

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

In my experience, instead of being afraid to break APIs, you can just put anything that isn't really stable inside of a unstable module. That way people (like me) who are ok depending on unstable features can do so without patching your library. Anyone who cares about that can avoid the module. This, to me, is the best of both worlds.

Copy link
Author

Choose a reason for hiding this comment

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

The problem with the top-level parsers is that you can't compose them yourself. If I have call-data inside of a larger blob, it's harder to use your parser (although I can pull it out of the LateEof error in this case). But, for example, I had to manually write decode_call_data here because I couldn't compose that with the other parsers. If I had access to that directly I may not have patched this library (although you could argue it was a good thing that the patch got upstreamed).

@jsdw
Copy link
Collaborator

jsdw commented Nov 1, 2021

Thank you! It might take me a few days to get to looking at this, but it looks like something worth wrapping my head around :)

@elliot-u410
Copy link
Author

Thank you! It might take me a few days to get to looking at this, but it looks like something worth wrapping my head around :)

The SignedPayload (or SignerPayload, as it's called in polkadot-js) is just [call-data][signed-extensions][extra-signed]. You were already parsing call-data and signed-extensions so I just copied the code for signed-extensions and changed it to parse extra-signed. Then I refactored the code to make it easy to compose these various parsers together to get the full payload parser.

@jsdw
Copy link
Collaborator

jsdw commented Nov 1, 2021

Ah, I understand! Basically, you'd like to expose the ability to decode the payload that would then be signed; got it!

I had a chat with @insipx and I think we're arriving at a decision on an API for this, so if it's OK with you @Noch3, I'll push a couple of commits to this PR tommorow (most likely) to shuffle a couple of bits around and keep the API consistent and such, but I'll keep the functionality you've added here, so you'll be able to decode the pre-signed payloads :)

@elliot-u410
Copy link
Author

If these commits are blocked by the CLA, you may not want to build on them directly.

@insipx
Copy link
Contributor

insipx commented Nov 1, 2021

If these commits are blocked by the CLA, you may not want to build on them directly.

Have you signed CLA or is it just not working?

@elliot-u410
Copy link
Author

I'm working with my employer to sign the CLA. It might take a while.

@elliot-u410
Copy link
Author

If you want to reimplement this on your own I won't be even slightly offended. Just so long as I can use it. :D

@jsdw
Copy link
Collaborator

jsdw commented Nov 3, 2021

I've opened #66 as my attempt to take this code, plus the request for more information form your previous PR, and mash them together into something, and would like to get your feedback on it :)

@elliot-u410
Copy link
Author

Closing in favor of #66

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