Add GUIDELINES.md for marking abstract contracts

[ernestognw]

Fixes #4350

Description

This is one of our guidelines we use across the repository, so it's now formally mentioned in our GUIDELINES.md document

[changeset-bot]

⚠️ No Changeset found

Latest commit: b0c5d1e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[Amxx]

AFAIK, this is all our contracts with very few exceptions (proxies / timelockcontroller / vestingwallet / paymentsplitter / presets).
Should we change them all to abstract

[ernestognw]

AFAIK, this is all our contracts with very few exceptions (proxies / timelockcontroller / vestingwallet / paymentsplitter / presets). Should we change them all to abstract

You're right, the wording doesn't sound completely precise, however, I chatted about this with Fran and there's clearly a need for a guideline here, so rephrasing would be ideal.

The discussion came from AccessControlAdminRules since it's abstract, similar to AccessControl. I don't think we should change what we have but definitely have a guideline.

What about...?

Contracts not designed for standalone usage are usually marked abstract so they are required to be inherited from other contracts.

[Amxx]

What about...?

Contracts not designed for standalone usage are usually marked abstract so they are required to be inherited from other contracts.

Honestly I prefer the guidelines to be strict, and not open to interpretations. I like the way its phrased right now, but it also mean we should probably change how our contracts are defined.

[frangio]

we should probably change how our contracts are defined.

Are you suggesting we should do this now or in 5.0?

I feel we shouldn't do it before 5.0, even though nothing should change for the end user some other things may be affected and we may not see them now. We can add a comment in the guidelines saying not all the codebase follows this guideline yet.

[ernestognw]

Honestly I prefer the guidelines to be strict, and not open to interpretations. I like the way its phrased right now, but it also mean we should probably change how our contracts are defined.

I also don't like ambiguity, but I think it's fine since the guidelines will be all correctly implemented long-term, so I agree we shouldn't change it until 5.0.

Same happens right now with changesets (eg. they should not have the PR URL, but they do now because of older PRs. Will get fixed over time).

[ernestognw]

We agreed to keep this guideline but applying breaking changes only in 5.0

[Amxx]

I added the abstract keyword to some contract definitions.

The list of non abstract contract can be obtain by doing

grep -rl ^contract --exclude-dir=mocks contracts

I left the following ones non-abstract

• contracts/finance/VestingWallet.sol
• contracts/governance/TimelockController.sol
• contracts/metatx/MinimalForwarder.sol
• contracts/proxy/ERC1967/ERC1967Proxy.sol
• contracts/proxy/beacon/BeaconProxy.sol
• contracts/proxy/beacon/UpgradeableBeacon.sol
• contracts/proxy/transparent/ProxyAdmin.sol
• contracts/proxy/transparent/TransparentUpgradeableProxy.sol