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: Use correct registry when parsing extrinsic call arguments #323

Merged
merged 2 commits into from
Oct 21, 2020

Conversation

emostov
Copy link
Contributor

@emostov emostov commented Oct 21, 2020

closes #322

relates to polkadot-js/api#2753

Previously we were using the ApiPromise registry to decode OpaqueCalls. It turns out this registry only has the latest metadata and will not have historic metadata. So when trying to parse some historic Calls, the bit of code changed by this PR would fail because it had the incorrect metadata for decoding.

This PR introduces the use of the blocks registry, which should always have the correct metadata to decode the extrinsics within. In theory we could use the registry off of any type from one of the historic queries, but I think using the block's registry is the is the most intuitive and readable.

Other note is that I updated all deps to latest versions.

@@ -396,11 +396,12 @@ export class BlocksService extends AbstractService {
* @param argsArray array of `Codec` values
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we put doc-comment for block too?

@@ -414,7 +415,10 @@ export class BlocksService extends AbstractService {
*
* @param genericCall `GenericCall`
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@@ -363,11 +363,12 @@ export class BlocksService extends AbstractService {
* @param argsArray array of `Codec` values
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

and here

@@ -381,8 +382,11 @@ export class BlocksService extends AbstractService {
*
* @param genericCall `GenericCall`
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

block @ param doc comment

@insipx
Copy link
Contributor

insipx commented Oct 21, 2020

LGTM apart from the doc-comments I think

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

This works. It’s not pretty but it gets the job done. Left a couple of questions but feel free to merge.
My more general concern about unbounded recursion on the other hackfix-PR still stands but shouldn’t block this ofc.

const { sectionName, methodName, callIndex } = genericCall;
private parseGenericCall(
genericCall: GenericCall,
block: Block
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to pass around the registry rather than the block? It would clarify the intent a bit (maybe it’s more efficient too?). Without knowing the context I think I would have had to read a fair amount of code before figuring out what the second params is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change to just the registry. I actually did the whole block just for readability and to make it clear the registry is associated with the block

} else {
newArgs[paramName] = argument;
}
}
}

return {
method: `${sectionName}.${methodName}`,
callIndex,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the callIndex here? It’s a private method so I guess it can change at any time, but curious why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a mistake! Good catch!

@emostov emostov merged commit b4678e1 into master Oct 21, 2020
@emostov emostov deleted the zeke-correct-reg branch October 29, 2020 02:06
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.

None yet

4 participants