Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add a Slots library, and a transient variant of ReentrancyGuard #4955

Closed
wants to merge 27 commits into from

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Mar 13, 2024

Fixes #4205
Fixes #4888

Note:

  • Hardhat update to 2.21.0: Technically only 2.20.0 is needed to support cancun, but 2.21.0 includes EDR that improve test speed
  • hardhat.config.js: Add a new entry evm / evmVersion that affects both the compiler (targeted evm version) and the node (executed evm version)
  • foundry.toml: enable cancun (and set compiler to 0.8.24)

PR Checklist

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

@Amxx Amxx added this to the 5.1 milestone Mar 13, 2024
@Amxx Amxx requested a review from ernestognw March 13, 2024 14:40
Copy link

changeset-bot bot commented Mar 13, 2024

🦋 Changeset detected

Latest commit: 5a77bc5

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

Copy link

socket-security bot commented Mar 13, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/hardhat-ignore-warnings@0.2.11 Transitive: filesystem +15 3.77 MB frangio
npm/hardhat@2.21.0 environment, filesystem, network, shell Transitive: eval, unsafe +265 231 MB fvictorio

🚮 Removed packages: npm/hardhat-ignore-warnings@0.2.9, npm/hardhat@2.17.4

View full report↗︎

Copy link
Contributor

@allwin199 allwin199 left a comment

Choose a reason for hiding this comment

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

@Amxx There is a typo in writting it should be writing

@frangio
Copy link
Contributor

frangio commented Mar 13, 2024

Note that there is a typo in the library name, it should be "transient" and not "transcient".

@Amxx Amxx changed the title Add a TranscientStorage library, and a transcient variant of ReentrancyGuard Add a TransientStorage library, and a transient variant of ReentrancyGuard Mar 13, 2024
/**
* @dev Returns the transient slot \`slot\` as a \`${type}\`.
*/
function load${capitalize(type)}(bytes32 slot) internal view returns (${type} r) {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason in particular to make the return value different to StorageSlot? The values there are defined as:

 struct Uint256Slot {
    uint256 value;
}

I definitely think this syntax is better, but I'm wondering if we should prefer one syntax over the other.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AFAIK, Solidity supports structs in memory and in storage, but not transient ones. You can't mark it. Only thing we have is low level opcode access through assembly ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Capture d’écran du 2024-03-14 20-50-54

Copy link
Collaborator Author

@Amxx Amxx Mar 17, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor

@frangio frangio Mar 14, 2024

Choose a reason for hiding this comment

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

Wouldn't it be nicer to provide an API more similar to the StorageSlot library?

A family of UDVT (one for each underlying type) that encapsulate a transient location with load() and store() methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ernestognw @frangio Like this new approach?

@Amxx
Copy link
Collaborator Author

Amxx commented Mar 18, 2024

Note: StorageSlot is friendlier that the proposed Slot, because it supports operations sur as +=, |= or &=. It also supports non value types (because solidity is in charge of the code generation for reading/writting to the "slot", we just force its location).

A few question

  • I think the slot derivation function are super usefull (even for StorageSlot), but I'm wondering where to put them. Maybe they deserve a dedicated file.
  • I think adding sstore and sload doesn't add complexity (just file length), so I'd keep that for completeness, even though it somehow overlaps with StorageSlot.
  • Maybe Slot.sol needs a different name. Ideas?

@frangio
Copy link
Contributor

frangio commented Mar 18, 2024

I think adding sstore and sload doesn't add complexity (just file length), so I'd keep that for completeness, even though it somehow overlaps with StorageSlot.

I actually think it's better to remove those methods, and make the library purely about transient storage. It makes it a more sound abstraction for transient pointers, where there is only one correct dereferencing operation, and where it is not correct to cast into a normal storage pointer.

If those are removed TransientSlot may be a better name for the library. I think it's probably okay to leave the derivation methods in there since they are not necessary for storage slots? I'm not opposed to putting them in a separate file, but it would make it a little less convenient to use, since the current library only requires using TransientSlot for *.

Note that "derive" is the correct verb to use.

@Amxx Amxx changed the title Add a TransientStorage library, and a transient variant of ReentrancyGuard Add a Slots library, and a transient variant of ReentrancyGuard Mar 18, 2024
@Amxx
Copy link
Collaborator Author

Amxx commented Mar 18, 2024

THe derivation function have a use for normal storage, see the diff to contracts/utils/Arrays.sol

So if slot is only about transient ... I would move the derivation function out of it into something "generic"

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use transient storage in ReentrancyGuard ReentrancyGuard with transient opcodes TSTORE and TLOAD
4 participants