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 Region.storeNat32/loadNat32 and Region.storeInt32/loadInt32 #4335

Merged
merged 5 commits into from
Dec 20, 2023

Conversation

crusso
Copy link
Contributor

@crusso crusso commented Dec 19, 2023

bugfix: fix broken implementations of Region.loadNat32, Region.storeNat32, Region.loadInt32, Region.storeInt32 (#4335). The operations were writing the 32-bit vanilla representation of the Motoko values, not the decoded values.

Values previously stored with the broken 32-bit operations must be loaded with care.
If bit 0 is clear, the original value can be obtained by an arithmetic shift right by 1 bit.
If bit 0 is set, the value cannot be trusted and should be ignored (it encodes some transient address of a boxed value).

@crusso crusso requested a review from luc-blaeser December 19, 2023 18:10
@crusso crusso marked this pull request as ready for review December 19, 2023 18:10
Copy link

Comparing from c20e455 to 1aa6697:
In terms of gas, 1 tests regressed and the mean change is +0.5%.
In terms of size, no changes are observed in 5 tests.

Copy link
Contributor

@luc-blaeser luc-blaeser left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this.
Luckily, you found this issue.

@crusso crusso added the automerge-squash When ready, merge (using squash) label Dec 20, 2023
@mergify mergify bot merged commit fe03888 into master Dec 20, 2023
@mergify mergify bot deleted the claudio/region-bug branch December 20, 2023 09:40
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Dec 20, 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.

2 participants