Upgrade to OpenZeppelin V5

[andreivladbrg]

Closes #702

[PaulRBerg]

When introducing a large change like this, it is important to explain the changes so that the PR reviewer has context.

The change from _requireMinted to _requireOwned is obvious, but the one from _beforeTokenTransfer to _update is not. Could you please shed some light on it (by sharing the link to the relevant OpenZeppelin docs, at a minimum)?

[andreivladbrg]

but the one from _beforeTokenTransfer to _update is not. Could you please shed some light on it (by sharing the link to the relevant OpenZeppelin docs, at a minimum)?

Well, you created the issue sharing the relevant OpenZeppelin docs 😅, it says in the link shared all informations needed.

I am going to share it again: https://github.com/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.0.0

[PaulRBerg]

Yep, "sharing again" would have been useful in the PR comment so that I don't have to hop through two links and identify the ERC-721 changes.

[PaulRBerg]

Generally looks good to me except for that comment below.

Also: don't we need to change the pragma to >0.8.20?

Compiling the code doesn't work with v0.8.19 (see my foundry-multibuild).

[andreivladbrg]

don't we need to change the pragma to >0.8.20?

missed it, we should

[andreivladbrg]

I changed the pragma solidity to >=0.8.20, does it look good now? @PaulRBerg if yes, can you approve?

[PaulRBerg]

@andreivladbrg the PR looks good technically now but it might be worth reconsidering if we want to merge this on main because we've already signed the engagement contracts with the auditors.

[PaulRBerg]

And it might also be worth holding off to upgrading to OpenZeppelin v5 until it gets a little more established in the industry, potential bugs are fixed, etc.

[andreivladbrg]

we've already signed the engagement contracts with the auditors

as far as I know they'll start working on this in november, the change made in src is just refactoring the ERC721 hooks, we should ask them first, but i doubt it would be an issue

until it gets a little more established in the industry, potential bugs are fixed

agree with this

[PaulRBerg]

as far as I know they'll start working on this in november

That's correct but the engagement contracts specifically mention the v1.1.0 changes as specified in the CHANGELOG on main.

we should ask them first

we should

agree with this

Then maybe we should merge this PR on a staging branch

[PaulRBerg]

Changed the base branch to 2.2. Merging now.