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

Bump to latest scale-encode/decode/value and fix test running #1103

Merged
merged 8 commits into from
Aug 2, 2023

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Aug 2, 2023

Running tests locally failed because we were still explicitly passing incorrect paths, so I fixed that. I also bumped versions of scale-encode, scale-decode and scale-value and fixed anything that broke as a result, and changed the CI to download to substrate-node to mirror the changed binary name.

One change that was spotted here in a failing test: previously, scale_value::Value's would each come with a type ID of the type they decoded from, and this could be used to re-encode the value in the same way. This is no longer always possible, because we now "skip" over Compact types when decoding. It's a trade-off:

  • Previously, any compact encoded thing (value, struct, whatever) was decoded to a single number. This lost information about the structs etc on the way there, and decoding into matching structs would fail because of this.
  • Now, we preserve all of the struct (composite) types in the decoding, but there's no longer a sensible place for the "compact" type ID to live (each decoded struct etc has its own type ID).

I think this is fair enough; the Type IDs that are attached to values are meant to help with diagnostics and such more than anything, and values should be encoded, as other types, by providing a target type ID to aim them at.

@jsdw jsdw requested review from a team as code owners August 2, 2023 10:45
@jsdw jsdw changed the title Bump to latest scale-encode,decode,value and fix test running Bump to latest scale-encode/decode/value and fix test running Aug 2, 2023
@jsdw jsdw merged commit 2176ec9 into master Aug 2, 2023
8 checks passed
@jsdw jsdw deleted the jsdw-bump-deps branch August 2, 2023 12:55
@jsdw jsdw mentioned this pull request Aug 2, 2023
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