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

Use scale-encode and scale-decode, and enable zero-copy decoding via Visitor trait #25

Merged
merged 25 commits into from
Mar 13, 2023

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Feb 28, 2023

Part of a series:

  • scale-encode: new crate to handle encoding using type info.
  • scale-decode: handle decoding using type info.
  • !scale-value: update Value type to use scale-encode/decode.
  • subxt: Start taking advantage of these in Subxt.

This PR:

  • Leans on scale-encode to encode Values.
  • Update Visitor impl in new scale-decode and ensure Value also impls DecodeAsType and DecodeAsFields/EncodeAsFields where appropriate.

The net effect of this is that Values share much of their encode and decode logic with every other static type now.

@jsdw jsdw changed the title Use scale-encode and scale-decode Use scale-encode and scale-decode, and enable zero-copy decoding via Visitor trait Feb 28, 2023
@jsdw jsdw requested a review from a team March 2, 2023 16:40
encode_decode_check(123u32, Value::u128(123));
encode_decode_check(123u64, Value::u128(123));
encode_decode_check(123u128, Value::u128(123));
//// Todo [jsdw]: Can we test this if we need a TypeInfo param?:
Copy link
Member

Choose a reason for hiding this comment

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

anything you want fix in this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this has been around a while, so I think it's fine; these u256/i256 types are a pain in general to work with!

(hiding whitespace in the github UI def helps given the space to tab change, sorry about that!)

}
// For other types, skip our value past a 1-value composite and try again, else error.
_ => {
if value.len() == 1 {
Copy link
Member

@niklasad1 niklasad1 Mar 9, 2023

Choose a reason for hiding this comment

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

maybe you do something like to fix the unwrap:

let mut it = values.values();
if let Some(v) = it.next() {
   if it.peek().is_none() {
       return inner_val.encode_as_type_to(type_id, types, out);
   } 
}
Err(Error::new(ErrorKind::WrongShape { actual: Kind::Tuple, expected: type_id }))

otherwise, just change it to an expect it easier to follow

@@ -1,4 +1,5 @@
hard_tabs = true
hard_tabs = false
Copy link
Member

@niklasad1 niklasad1 Mar 9, 2023

Choose a reason for hiding this comment

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

👀

are you tired of tabs? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's mainly just that subxt, scale-decode and scale-encode all use spaces :)

But also, a really annoying thing I run into with tabs is making doc examples; I can't remember the exact issue but something like the IDE tries to make things tabs but cargo complains about tabs in doc comments; either way it all "just works" with spaces I guess (and spaces also seem to be the default anyway)

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

looks clean

Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work! 👍

@jsdw jsdw merged commit 550654a into main Mar 13, 2023
@jsdw jsdw deleted the jsdw-decode-as-type branch March 13, 2023 13:44
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