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

Support serialization of fixed-size arrays in abi_serializer. #1918

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

greg7mdp
Copy link
Contributor

@greg7mdp greg7mdp commented Nov 18, 2023

[partially] resolves #1931.

  • add support for serialization of fixed size array types such as uint8[5].
  • [perf] add missing reserve() when deserializing a vector (or now an array), fix unnecessary copy in vector<fc::variant> vars = var.get_array();
  • add tests (thanks to Kevin) for serialization of fixed size array types.

@greg7mdp
Copy link
Contributor Author

I think we should check that the length of the array passed to serialize matches the size in the rtype.

libraries/chain/abi_serializer.cpp Outdated Show resolved Hide resolved
libraries/chain/abi_serializer.cpp Outdated Show resolved Hide resolved
libraries/chain/abi_serializer.cpp Show resolved Hide resolved
libraries/chain/abi_serializer.cpp Outdated Show resolved Hide resolved
unittests/abi_tests.cpp Outdated Show resolved Hide resolved
…lues.

Also replace the mouthful `abi_serializer::create_yield_function( max_serialization_time )` with `yield_fn()`
@greg7mdp greg7mdp requested a review from spoonincode November 18, 2023 22:00
@spoonincode
Copy link
Member

I'm missing some context to this change so I may be misunderstanding what the goal is here. Fixed sized arrays have (annoyingly imo) been an omission of the ABI spec. My first thought is if cdt is generating a uint8_t[96] type in an ABI, it's generating a defective ABI. Such an ABI would be disallowed in, for example, abieos,
https://github.com/AntelopeIO/abieos/blob/main/src/abi.cpp#L58
and we'd have to investigate eosjs, wharf, etc too. Are we trying to add support in the ABI spec for type[size]?

@greg7mdp
Copy link
Contributor Author

Are we trying to add support in the ABI spec for type[size]?

I'll let Kevin answer this one. Regardless of whether fixed-size arrays are officially supported in the abi, I don't think it would hurt to have these supported in abi_serializer.

@greg7mdp greg7mdp marked this pull request as draft November 27, 2023 20:28
@greg7mdp greg7mdp linked an issue Nov 27, 2023 that may be closed by this pull request
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.

Support serialization of fixed-size arrays in abi_serializer.
3 participants