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

Consider changing memory-safe NatSpec annotations to assembly('memory-safe') dialect string #4971

Closed
Tracked by #5249
ernestognw opened this issue Mar 25, 2024 · 4 comments
Milestone

Comments

@ernestognw
Copy link
Member

📝 Details

As pointed out by @ZumZoom in #4941 (comment):

Should it still be a case though? It seems that after merging #4288 there is little sense in sticking with comment annotation. assembly ("memory-safe") was introduced in 0.8.13 and current pragma on all the contracts is ^0.8.20.

Given that we're not supporting any Solidity older than 0.8.20, it makes sense to reconsider changing the NatSpec annotations to the new syntax added in Solidity 0.8.13:
ethereum/solidity#12620

@ZumZoom
Copy link
Contributor

ZumZoom commented Mar 25, 2024

Regarding the benefits. The only benefit I see is getting ready for solidity 0.9. As stated in the last paragraph of memory-safety conventions docs solidity devs are going to remove the support of NatSpec annotations in the next breaking release.

@ernestognw ernestognw added this to the 5.x milestone Mar 25, 2024
@ernestognw
Copy link
Member Author

Right, so we'll have to make this change before 9.x is released. Not sure when that's going to happen though but I'm adding the 5.x milestone so we tackle it eventually.

@Amxx
Copy link
Collaborator

Amxx commented Mar 26, 2024

With 0.9, I expect we'll have a lot more than that to change. I would not consider "preparing the codebase for 0.9" something we have to do know.

I do prefer the propose notation over natspec. It feels more natural to me to mark the assembly block directly... but that is very personnal.

@ernestognw
Copy link
Member Author

This issue came during the audit and we sorted it out on #5172.

Closing

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

No branches or pull requests

3 participants