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

Refactor access to Checkpoint struct without using memory #4512

Merged

Conversation

0xVolosnikov
Copy link
Contributor

@0xVolosnikov 0xVolosnikov commented Aug 5, 2023

Fixes mentioned in:
#4495 (comment)

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@changeset-bot
Copy link

changeset-bot bot commented Aug 5, 2023

⚠️ No Changeset found

Latest commit: 3a57b7d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@0xVolosnikov
Copy link
Contributor Author

0xVolosnikov commented Aug 5, 2023

I would also like in this PR to slightly change some lines in:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/Checkpoints.sol

Since in this contract the structure (-s) is repeatedly written to the memory. Also, I suspect that such structure reading can create an overhead (since self is read multiple times).

However, this may require changing relatively many lines of code, and I'm not sure that such changes are desired for version 5.0. So I would like to hear the opinion of the maintainers on this. I'm not sure if it's worth creating a separate issue for this, but if necessary, I can create one.

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Looks good, gas is slightly impproved.

@frangio
Copy link
Contributor

frangio commented Aug 25, 2023

I would also like in this PR to slightly change some lines in: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/Checkpoints.sol

Since in this contract the structure (-s) is repeatedly written to the memory. Also, I suspect that such structure reading can create an overhead (since self is read multiple times).

However, this may require changing relatively many lines of code, and I'm not sure that such changes are desired for version 5.0. So I would like to hear the opinion of the maintainers on this. I'm not sure if it's worth creating a separate issue for this, but if necessary, I can create one.

Open to this experiment if you want to give it a shot, but agree with doing it for 5.1 since we're now focused on quality control for 5.0. Feel free to open a PR though we may just keep it open until post-5.0.

@frangio frangio merged commit a5ed318 into OpenZeppelin:master Aug 25, 2023
16 of 17 checks passed
@0xVolosnikov 0xVolosnikov deleted the optimize-checkpoints-usage branch August 26, 2023 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants