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

Add Warning about use of delete #3412

Merged
merged 4 commits into from May 18, 2022

Conversation

lucasAlonso
Copy link
Contributor

@lucasAlonso lucasAlonso commented May 15, 2022

Fixes #3400

Added Warning about delete operator on structs like EnumerableSet and EnumerableMap even though they contain a mapping that will not be cleared, and the operation will in fact corrupt the struct's data.

According to ethereum/solidity#11843 this operation will no longer be allowed in Solidity 0.9, but in the meantime we should document that delete should not be used with these types.

as user frangio stated, i checked the info and link

PR Checklist

  • Documentation
    check if this Warning is useful and apply.
    Need any further warning or issue?

@JulissaDantes JulissaDantes changed the title Added Warning about use of delete Add Warning about use of delete May 16, 2022
@Amxx
Copy link
Collaborator

Amxx commented May 16, 2022

Hello @lucasAlonso

While I do agree with the need for a warning, I'm not sure this is the right place to put it.

IMO this should be at the level of the solidity library (top part of the file).

@lucasAlonso
Copy link
Contributor Author

Hi Amxx,
at the issue opened (#3400), @SamuelOsewa said
"I believe that it should be when the delete is used so if someone wants to see the use case they would see the comment as people tend to skip over the introduction."
Maybe two warnings? or add a more general issue that involves change delete for clear
This is my first PR, so sorry if I'm overthinking :)

@Amxx
Copy link
Collaborator

Amxx commented May 16, 2022

It is important for this warning to show up in the documentation. For that to happen, it must be part of the natspec comments.

@lucasAlonso
Copy link
Contributor Author

Amxx changed Warning placement, now at top part of the file.
Thxs!

* // WARNING!
* // delete function should not be used. It will corrupt the data structure,
* // clear function should be used instead.
* // ref https://github.com/ethereum/solidity/pull/11843
Copy link
Collaborator

Choose a reason for hiding this comment

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

Format is not correct.

See utils/Address.sol for how to properly format a warning.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, delete in not a "function", and there is no clear function in this structure.

I would suggest something like:

Trying to delete such a structure from storage will likely result in data corruption, rendering the structure unusable.
See ethereum/solidity#11843 for more info

In order to clean an EnumerableMap, you can either remove all elements one by one or create a fresh instance using an array of EnumerableMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed wording and format.
maybe add an issue at contracts/utils/cryptography/MerkleProof.sol to make Warning's Format consistent ?

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
@Amxx Amxx enabled auto-merge (squash) May 18, 2022 17:16
@Amxx Amxx disabled auto-merge May 18, 2022 17:16
@Amxx Amxx enabled auto-merge (squash) May 18, 2022 17:16
@Amxx Amxx merged commit c2077f0 into OpenZeppelin:master May 18, 2022
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.

Document limitation of delete for EnumerableSet and Map
3 participants