-
Notifications
You must be signed in to change notification settings - Fork 431
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 decode_all
for decoding cross contract call result
#1810
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.
Looks good, but why can't we have the same generic integration test for the delegate call?
Codecov Report
@@ Coverage Diff @@
## master #1810 +/- ##
==========================================
+ Coverage 52.11% 52.16% +0.04%
==========================================
Files 206 206
Lines 6656 6656
==========================================
+ Hits 3469 3472 +3
+ Misses 3187 3184 -3 see 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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!
Related to GHSA-853p-5678-hv8f.
decode
can still succeed if there are bytes remaining. In the above security advisory the issue was that the bytes forResult<T>
were successfully decoded into aT
but with the incorrect value, becauseResult
encoding has an extra byte prefix.If we had been using
decode_all
instead, which fails if there are any bytes remaining in the input, this would have been discovered earlier.This provides an extra level of safety since it avoids e.g. truncation of values: preventing e.g. an
i32
returned by the callee being decoded into ani8
(seeintegration-test
as part of this PR).Note there are other uses of
decode
which could be replaced bydecode_all
(see #1804), but we can tackle those separately.