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

bugfix: fix candid decoding at immutable array types to support opt defaulting #4240

Merged
merged 29 commits into from
Oct 13, 2023

Conversation

crusso
Copy link
Contributor

@crusso crusso commented Oct 9, 2023

Another Candid decoding bug, seems independent of #4238.

I think the code for decoding immutable arrays was never extended to handle backtracking correctly.
When recovery is an option (haha) and the type descriptor does not match, the current code would continue to try to decode the array length and elements. When recovery is not an option, the previous trap would render this code unreachable. But when recovery is enabled, we need to bail and not try to read the array size and elements but just return the sentinel value instead.

  • Consider applying the same refactoring to mutable array, if possible.
    Since these aren't Candid, but extended Candid (for stable variables), and don't need to support recovery, the change is (probably) not necessary but nice for uniformity only.
  • restore or delete original repro - I must have edited it while experimenting

@crusso crusso changed the title experiment: possibly yet another idl decoding bug bugfix: fix candid decoding at immutable array types to support opt defaulting Oct 10, 2023
@crusso crusso changed the base branch from master to claudio/idl-variant-fix October 10, 2023 22:16
test/run/idl-decoding-bug.mo Outdated Show resolved Hide resolved
test/run/idl-opt-tests.mo Outdated Show resolved Hide resolved
@crusso crusso marked this pull request as ready for review October 10, 2023 22:26
@github-actions
Copy link

github-actions bot commented Oct 10, 2023

Comparing from 2d3c4de to b0c1f12:
In terms of gas, 1 tests improved and the mean change is -2.1%.
In terms of size, no changes are observed in 5 tests.

src/codegen/compile.ml Outdated Show resolved Hide resolved
src/codegen/compile.ml Outdated Show resolved Hide resolved
src/codegen/compile.ml Outdated Show resolved Hide resolved
Base automatically changed from claudio/idl-variant-fix to master October 12, 2023 09:56
src/codegen/compile.ml Outdated Show resolved Hide resolved
@crusso crusso requested a review from ggreif October 13, 2023 10:41
Copy link
Contributor

@ggreif ggreif left a comment

Choose a reason for hiding this comment

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

Rubber-stamping!

Changelog.md Outdated Show resolved Hide resolved
@crusso crusso added the automerge-squash When ready, merge (using squash) label Oct 13, 2023
@mergify mergify bot merged commit 80e46a5 into master Oct 13, 2023
8 checks passed
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Oct 13, 2023
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.

3 participants