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

fix: correctly serialize a multsig call, then parse WrapperKeepOpaque, and WrapperOpaque #840

Merged
merged 4 commits into from
Feb 15, 2022

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Feb 10, 2022

closes: #835

This PR resolves serializing a MultiSig call and correctly parsing the args with the added WrapperKeepOpaque, and WrapperOpaque types. The code is a little repetitive to keep it readable, and understandable, also parseGenericCalls is heavily leveraging recursion so best to not mess with the logic much.

I added an e2e test for the block mentioned in the issue to ensure we are tracking a WrapperKeepOpaque call.

This is via polkadot-js/api v7.6.1

@TarikGul TarikGul requested a review from a team as a code owner February 10, 2022 04:09
src/services/blocks/BlocksService.ts Outdated Show resolved Hide resolved
src/services/blocks/BlocksService.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

LGTM, not a big fan of the repetition since it is identical even at a type level but up to you :)

@TarikGul
Copy link
Member Author

@jsdw I made the changes to stop the repetition, i totally agree that the solution you provided was way better.

)
) {
// multiSig.asMulti.args.call is either an OpaqueCall (Vec<u8>),
// WrapperKeepOpaque<Call>, or WrapperOpaque<Call> that we
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit hard to map the code in the try…catch block to each of the three cases. What do you think about something like this:

			} else if (
				argument &&
				paramName === 'call'
			) {
				switch (argument?.toRawType()) {
					case 'Bytes':
					case 'WrapperKeepOpaque<Call>':	
						newArgs[paramName] = argument;
					case 'WrapperOpaque<Call>':
						const call = registry.createType('Call', argument.toHex());
						newArgs[paramName] = this.parseGenericCall(call, registry);
	
					default:
						// log or otherwise report error?;
				}

(Not sure I got the cases right, which is illustrating the point I guess)

Copy link
Member

Choose a reason for hiding this comment

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

I like that if that works :)

Copy link
Member Author

Choose a reason for hiding this comment

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

So actually each of the 3 cases are going to map out to the same output i.e.

					try {
						const call = registry.createType('Call', argument.toHex());
						newArgs[paramName] = this.parseGenericCall(call, registry);
					} catch {
						newArgs[paramName] = argument;
					}

therefore the above wont work. I filed an issue here regarding the try catch in order to potentially optimize it. For this PR i think it is out of scope as we are just extending the conditional to include WrapperKeepOpaque<Call>, and WrapperOpaque.

@TarikGul
Copy link
Member Author

Merging. @dvdplm I think your comment has some merit above, and should be given some extra investigation and potential optimization. That being said, for the scope of this PR, lets just patch the bug (since the solution itself is pretty simple and doesnt change any of the core logic when parsing the generic calls). But I will follow this PR up with the issue I filed and see what we can do :)

@TarikGul TarikGul merged commit 60826c0 into master Feb 15, 2022
@TarikGul TarikGul deleted the tarik-wrapper-opaque branch February 15, 2022 21:16
This pull request was closed.
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.

v11.3.8 not parse call parameters
5 participants