Skip to content

Conversation

@Amxx
Copy link
Collaborator

@Amxx Amxx commented Sep 5, 2023

Fixes LIB-1041

PR Checklist

  • Changeset entry (run npx changeset add)

@Amxx Amxx requested a review from frangio September 5, 2023 13:49
@changeset-bot
Copy link

changeset-bot bot commented Sep 5, 2023

🦋 Changeset detected

Latest commit: 175b1e9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

Not sure what this means? Click here to learn what changesets are.

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

if (!initialSetup && !construction) {
revert AlreadyInitialized();
}
$._initialized = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

$._initialized = 1;
$._initializing = isTopLevelCall;

May be cheaper here, removing redundant SLOAD operation in case of !isTopLevelCall

Copy link
Collaborator Author

@Amxx Amxx Sep 5, 2023

Choose a reason for hiding this comment

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

I'm not sure I understand your proposal. Before doing _;, we want to make sure the initializing is true, this would possibly invert it to false.

Also I'm not sure where the duplicated sload is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I'm not sure where the duplicated sload is.

Since $ is a struct with two fields packed in one storage slot, compiler need to load this slot if you change only one field. If you write all the fields of the structure at once, the compiler does not need to read this slot before SSTORE

Copy link
Contributor

@0xVolosnikov 0xVolosnikov Sep 5, 2023

Choose a reason for hiding this comment

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

So it could (in theory) save around 100 gas here

$._initialized = 1;
if (isTopLevelCall) {
$._initializing = true;
}

If do it like

$._initialized = 1;
$._initializing = isTopLevelCall;

Copy link
Contributor

@0xVolosnikov 0xVolosnikov Sep 5, 2023

Choose a reason for hiding this comment

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

this would possibly invert it to false

Upd: yes, in this case my proposal should be rather:

  $._initialized = 1;
  $._initializing = true;

And here:

$._initialized = 1;
$._initializing = false;

Copy link
Contributor

Choose a reason for hiding this comment

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

Writing it that way requires proving that the _; part doesn't change the value of $._initialized.

I think it is true, because $._initializing = true prevents that value from changing in the rest of the contract, but it's more difficult to see.

I think I prefer to keep the current code at this stage (audit fixes) and come back to this for a future release. @vladyan18 can you open an issue?

@frangio frangio merged commit 5abbd04 into OpenZeppelin:audit/wip/2a-2b Sep 5, 2023
@Amxx Amxx deleted the refactor/Initializable/intermediate-variables branch September 6, 2023 09:35
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