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

Add bitvec-like generic support to the scale-bits type for use in codegen #718

Merged
merged 16 commits into from
Nov 24, 2022

Conversation

Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Nov 17, 2022

Fixes #693

Hopefully this is in line with what was expected from #693 cc @jsdw

Comment on lines 194 to 196
// NOTE: We could reduce allocations if it would be possible to directly
// decode from an `Input` type using a custom format (rather than default <u8, Lsb0>)
// for the `Bits` type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It definitely feels from this PR that we could improve scale_bits::decode_using_format_from to work with that Input trait rather than accepting an array of bytes; it'd be nice to be able to condense all of this decode logic into a few lines much like the encode version below, and the only reason that you can't is that the interface in scale-bits isn't quite right :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Opened an issue for this here: paritytech/scale-decode#10

subxt/src/utils.rs Outdated Show resolved Hide resolved
subxt/src/utils.rs 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.

Looks great; love the approach and well commented (shame that scale-bits doesn't provide the right interface to help with decoding, so we should fix that). Just a wee tweak and perhaps some tests to make sure that our encoding/decoding lines up with BitVec still to check compatibility.

Maybe with those tests especially it becomes worth turning utils into a folder and moving this bits to a separate file?

subxt/src/utils.rs Outdated Show resolved Hide resolved
@Xanewok Xanewok requested a review from jsdw November 21, 2022 16:55
subxt/src/utils/mod.rs 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.

Looks great! One tiny nit about util::bits imports but great stuff!

subxt/src/utils/bits.rs Outdated Show resolved Hide resolved
subxt/src/utils/bits.rs Outdated Show resolved Hide resolved
subxt/src/utils/bits.rs Outdated Show resolved Hide resolved
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.

nice work, looks good

I have some minor comments which are optional to fix :P

@Xanewok Xanewok merged commit f0ce26d into master Nov 24, 2022
@Xanewok Xanewok deleted the igor-prune-bitvec branch November 24, 2022 14:09
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.

Remove bitvec from subxt-codegen
3 participants