-
Notifications
You must be signed in to change notification settings - Fork 123
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
Feature/sbor encoder decoder traits #597
Conversation
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.
Overall looks good to me.
The wasm size increase is a bit concerning - primarily due to extra logic to the deal with result and unwrap.
The error propagation is nice but I wonder if it makes more sense to make a comprise by treating SizeTooLarge
and MaxDepthExceeded
invariants as majority of the clients are just unwrapping/expecting anyway. Panics!
EncodeError
propagation is usefull when the system creates/transforms/encloses "any" value based on user inputs, and attempts to encode.
Not a blocker.
This clarifies in the name a distinction from the more standard direct calls which can be made against a type/value.
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.
LGTM!
As per discussions with Yulong, this PR implements changes to the Encode/Decode and Encoder/Decoder traits, including now returning an
EncodeError
.There needs to be symmetry between Encode and Decode to ensure that there aren't any attack differentials where (eg) we save a substate but can't read it back... I'd appreciate if during code review we have another pair of eyes on this.
Some Asides
I originally set the max SBOR recursion depth to 32, but this choked some tests (where they failed to store a Package substate due to the recursion in some of the ABIs) as they needed a depth of 34 to pass. For now, I've gone with 64 - which seems much less likely to choke in non-malicious situations. When we linearise schemas, the ABI depth won't be a problem.
With the new encoder, I was also hitting WASM native stack depth limits during creation of the ABI when the ABI contained a deeply nested structure such as ComponentInfoSubstate containing AuthRules. My solution has been to raise stack depths from 512 to 1024 across the board, as if we can hit them in normal use case, it feels a bit wrong. Does this feel okay? (This was incredibly hard to debug because it just returned a WASM panic... Heads up for anyone hitting weird panics in future. Also note that the
In terms of WASM size for (eg) the
faucet.wasm
I'm not really sure why the code has grown by so much, even with the infallible branches removed - Yulong, might be interesting to take a look at the WASM to see if I've made a mistake and included code accidentally or something 🤷♂️
Still to come in another PR:
Breaking Changes:
Decode<X>
trait has changed toDecode<X: CustomTypeId, D: Decoder<X>>
to allow swapping out the decoder (eg to support other decoder implementations in future - eg streaming decoding)Encode<X>
trait has changed toEncode<X: CustomTypeId, E: Encoder<X>>
to allow swapping out the encoder (eg to support other encoder implementations in future - eg streaming encoding; or throwing errors if too much space is used)Result<(), EncodeError>
, with encode errors for exceeding the 32 depth or for the max size being exceeded.encode_any
,encode_any_with_buffer
anddecode_any
have been removed - instead, you can simply encode/decode an SborValue or scrypto_encode/scrypto_decode a ScryptoValueScryptoEncode
,ScryptoDecode
andScryptoTypeId
have been introduced. These can be thought of as "aliases" which should be used for arguments, and have a blanket implementation from the fundamentalEncode
/Decode
/TypeId
traits. These fundamental traits should be used for any implementations.scrypto_encode
(and similar functions) now returnResult<Vec<u8>, EncodeError>