Skip to content

Fix type enumeration for stripped metadata in subxt.#23

Merged
jsdw merged 5 commits intomasterfrom
tadeohepperle/type-enumeration-bugfix
Mar 25, 2024
Merged

Fix type enumeration for stripped metadata in subxt.#23
jsdw merged 5 commits intomasterfrom
tadeohepperle/type-enumeration-bugfix

Conversation

@tadeohepperle
Copy link
Copy Markdown
Contributor

I discovered that when retaining only parts of the metadata in subxt, we can end up with types where their id and their index in the Vec they are held in do not line up.
This caused the ui-tests to fail on the subxt core PR, see this action run.

I verified locally that with this change the UI-tests succeed again in subxt.
Though I suspect that there is a deeper problem somewhere and that we actually do not want to have the index and id of types to differ anywhere.

Let's just push this fix.

@jsdw
Copy link
Copy Markdown
Collaborator

jsdw commented Mar 22, 2024

When you .retain() only certain types in a PortableRegistry, it is very likely that their IDs will be changed (the IDs are just indexes into the registry Vec, so to remove types you inevitably have to change some IDs. Is this changing of ID what you are referring to here?

On the other hand, if you call .retain() and you run into a situation where some IDs in the registry are now invalid or point to different types than they used to, then that's probably a bug in https://github.com/paritytech/scale-info/blob/1e02764a1b6a59ef9e977750f2a60947d7dd47d1/src/portable.rs#L85.

@tadeohepperle
Copy link
Copy Markdown
Contributor Author

tadeohepperle commented Mar 22, 2024

There might be a bug in scale-info, I just noticed that in for (ty_idx, ty) in types.types.iter().enumerate(), the ty_idx and ty.id are not always the same. I feel like they should be though, even after retaining.


// First, group types if they are similar (same path, same shape).
for (ty_idx, ty) in types.types.iter().enumerate() {
// Note: ty_idx and ty.id might be different
Copy link
Copy Markdown
Collaborator

@jsdw jsdw Mar 22, 2024

Choose a reason for hiding this comment

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

Hmm, I don't think that should be the case, but maybe we have never really checked that ID variable.

I can see in scale_info::PortableRegistry::retain that we don't seem to update that type ID to reflect where its new location in the vec will be (https://github.com/paritytech/scale-info/blob/1e02764a1b6a59ef9e977750f2a60947d7dd47d1/src/portable.rs#L115) so probably we should fix that (unless @lexnv you can remember why we wouldn't have updated that Id too?).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's point this comment to your scale-info PR to fix it and make clear it's a bug :)

if types_equal_extended_to_params(&ty.ty, ty_b) {
group.push(ty.id);
let other_ty_in_group_idx = group[0]; // all types in group are same shape; just check any one of them.
let other_ty_in_group = types.types.get(other_ty_in_group_idx).expect("ty exists");
Copy link
Copy Markdown
Collaborator

@jsdw jsdw Mar 22, 2024

Choose a reason for hiding this comment

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

might as well use types.resolve() here I guess? all it does is a get under the hood and then gives back .ty :)

Copy link
Copy Markdown
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!

@jsdw jsdw merged commit 64ec335 into master Mar 25, 2024
@jsdw jsdw deleted the tadeohepperle/type-enumeration-bugfix branch March 25, 2024 14:00
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