-
Notifications
You must be signed in to change notification settings - Fork 33
Add support for fixed size arrays in abi_serializer. #1614
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
Conversation
|
Noticed that the type for spring/libraries/libfc/include/fc/io/varint.hpp Lines 91 to 99 in 4879600
|
| set(std::move(key), variant( val ) ); | ||
| return std::move(*this); | ||
| } | ||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests for new variant functionality to test_variant.cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compatibility with previous versionsSupporting fixed size array did not require any change in as opposed to Also, in the abi, Spring was lacking support for the fixed size array abi type in ex1: passing a std::array as parameter: ex2: action taking a std::vector and returning a std::array: Note that the displayed return value is incorrect (should be However, the issue is only in I have verified that if the hex data passed to the action ( So the changes in this PR only allow |
This is for pre-2.0 versions of |
Yes, exactly. Using the version from this PR all works fine with cleos (passing array parameters, and returning them, whether these are variable or fixed size arrays in the abi). |
Presumably it is the version of |
Actually it will be version of |
I think it is always the version of |
The |
|
After discussion in standup, rebasing this PR to |
| "base": "", | ||
| "fields": [{ | ||
| "name": "a", | ||
| "type": "uint8[5]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like uint8[]$ is considered a valid type currently, so probably should have a test for something like uint8[5]$. Should uint8[5]? be allowed? I'd say make either a positive or negative test for that too depending on what we want (it kinda feels like it ought to be allowed but I'm not sure I have an immediate strong opinion on it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it should be allowed as well. I'll add a test for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, actually, neither abi_serializer nor abieos seem to support optional vectors (var size arrays).
It doesn't seem like it would be too hard to fix though.
However, to do it properly would require some kind of recursion though, as a type can be T?, and T can be T2[], and T2 can be T3?, so we cannot expect to get to the fundamental_type without recursion as we do today in:
spring/libraries/chain/abi_serializer.cpp
Lines 252 to 262 in 5227aa3
| std::string_view abi_serializer::fundamental_type(const std::string_view& type)const { | |
| if( is_array(type) ) { | |
| return type.substr(0, type.size()-2); | |
| } else if (is_szarray (type) ){ | |
| return type.substr(0, type.find_last_of('[')); | |
| } else if ( is_optional(type) ) { | |
| return type.substr(0, type.size()-1); | |
| } else { | |
| return type; | |
| } | |
| } |
If we want to do that I think it probably should be in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about uint8[5]$ though? Pretty sure uint8[]$ works today (at least, there looks like a test for it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spoonincode
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend updating the wiki doc specifying that only type[n]$ is allowed, not type[n]?. Maintainers of other implementations might have the same uncertainty during development.
I still think it would be useful to have a test that verifies type[n]? is sanely rejected and doesn't do anything funky.
But it wasn't clear to me if you were intending to tackle type[n]? later anyways. So maybe the 2 comments above doesn't apply
…) are rejected by the abi.
|
I added a sentence in the limitiations of the fixed size array wiki: https://github.com/AntelopeIO/spring/wiki/ABI-1.3:-Fixed-sized-arrays. Also added tests for I'm happy to add support for |
|
oh don't forget to add [2.0.0-dev] to the PR
idk if the juice is worth the squeeze. I was more interested in us being clear what was and was not expected to work |
actually wait, why is this going to main right now? |
We talked about it in standup last week, and Areg asked me to move it back to main because we chose to consider that cdt had correct support for a while, and the BTW don't know if you noticed, warfkit also fixed their support for fixed size arrays wharfkit/antelope#119 |
|
I completely disagree but I'm not going to debate it any more |
Resolves #1401.
Replaces AntelopeIO/leap#1918
action examples
generated abi (current
cdtwith no change)executing the action
testrev(vector version)executing the action
testre(fixed size array version)