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

Enable partial transpilation for upgradeable package #4628

Merged
merged 36 commits into from
Sep 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
0f54291
enable partial transpilation
frangio Sep 27, 2023
b4a9a76
refactor prepare/prepack scripts
frangio Sep 27, 2023
f648bfe
add vanilla as dependency during upgradeable tests
frangio Sep 27, 2023
3910fbc
fix npm pack output
frangio Sep 27, 2023
270c006
fix npm pack
frangio Sep 27, 2023
df08dd7
fix transpile command
frangio Sep 27, 2023
59746e6
change approach without using pack
frangio Sep 27, 2023
e904e02
fix workflow
frangio Sep 27, 2023
0f2bafe
fix checkout location
frangio Sep 27, 2023
8002e8d
fix run
frangio Sep 27, 2023
9e2dcf1
fix mkdir
frangio Sep 27, 2023
0ff2901
fix ln
frangio Sep 27, 2023
5d8422f
add transpiler as dependency]
frangio Sep 27, 2023
de7a277
update hardhat-exposed
frangio Sep 27, 2023
63acb32
update transpiler
frangio Sep 27, 2023
5f0c801
enable exposed imports
frangio Sep 27, 2023
b6476c1
fix config
frangio Sep 27, 2023
448d8f6
add Stateless.sol
frangio Sep 27, 2023
0f7c3e2
fix stateless
frangio Sep 27, 2023
7246dbb
add dummy contract in stateless file
frangio Sep 27, 2023
ef88118
add all library imports
frangio Sep 27, 2023
01a768b
update hardhat-exposed
frangio Sep 27, 2023
e16c0ae
mark some contracts as stateless
Amxx Sep 28, 2023
9dc5442
sort
Amxx Sep 28, 2023
4771215
Add peer dependency to package.json
Amxx Sep 28, 2023
4ca92e2
fix import path
Amxx Sep 28, 2023
b320373
cleanup
Amxx Sep 28, 2023
268e17f
mark UUPSUpgradeable as stateless
Amxx Sep 28, 2023
781d5f2
Merge branch 'master' into partial-transpilation
frangio Sep 28, 2023
242b716
add changeset
frangio Sep 28, 2023
8d8030b
Update .github/workflows/checks.yml
frangio Sep 28, 2023
f63cf2d
simplify prepack
frangio Sep 28, 2023
86ebb2e
fix order of transpilation
frangio Sep 28, 2023
9e1ce8d
Merge branch 'master' into partial-transpilation
frangio Sep 28, 2023
b51a866
fix location where peer dependency is added
frangio Sep 28, 2023
5336454
read package version from contracts/package.json
Amxx Sep 28, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/grumpy-poets-rush.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': major
---

Upgradeable Contracts: No longer transpile interfaces, libraries, and stateless contracts.
2 changes: 0 additions & 2 deletions .github/actions/setup/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,3 @@ runs:
run: npm ci
shell: bash
if: steps.cache.outputs.cache-hit != 'true'
env:
SKIP_COMPILE: true
3 changes: 3 additions & 0 deletions .github/workflows/checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ jobs:
fetch-depth: 0 # Include history so patch conflicts are resolved automatically
- name: Set up environment
uses: ./.github/actions/setup
- name: Copy non-upgradeable contracts as dependency
run:
cp -rnT contracts node_modules/@openzeppelin/contracts
- name: Transpile to upgradeable
run: bash scripts/upgradeable/transpile.sh
- name: Run tests
Expand Down
36 changes: 36 additions & 0 deletions contracts/mocks/Stateless.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;

// We keep these imports and a dummy contract just to we can run the test suite after transpilation.

import {Address} from "../utils/Address.sol";
import {Arrays} from "../utils/Arrays.sol";
import {AuthorityUtils} from "../access/manager/AuthorityUtils.sol";
import {Base64} from "../utils/Base64.sol";
import {BitMaps} from "../utils/structs/BitMaps.sol";
import {Checkpoints} from "../utils/structs/Checkpoints.sol";
import {Clones} from "../proxy/Clones.sol";
import {Create2} from "../utils/Create2.sol";
import {DoubleEndedQueue} from "../utils/structs/DoubleEndedQueue.sol";
import {ECDSA} from "../utils/cryptography/ECDSA.sol";
import {EnumerableMap} from "../utils/structs/EnumerableMap.sol";
import {EnumerableSet} from "../utils/structs/EnumerableSet.sol";
import {ERC1155Holder} from "../token/ERC1155/utils/ERC1155Holder.sol";
import {ERC165} from "../utils/introspection/ERC165.sol";
import {ERC165Checker} from "../utils/introspection/ERC165Checker.sol";
import {ERC1967Utils} from "../proxy/ERC1967/ERC1967Utils.sol";
import {ERC721Holder} from "../token/ERC721/utils/ERC721Holder.sol";
import {Math} from "../utils/math/Math.sol";
import {MerkleProof} from "../utils/cryptography/MerkleProof.sol";
import {MessageHashUtils} from "../utils/cryptography/MessageHashUtils.sol";
import {SafeCast} from "../utils/math/SafeCast.sol";
import {SafeERC20} from "../token/ERC20/utils/SafeERC20.sol";
import {ShortStrings} from "../utils/ShortStrings.sol";
import {SignatureChecker} from "../utils/cryptography/SignatureChecker.sol";
import {SignedMath} from "../utils/math/SignedMath.sol";
import {StorageSlot} from "../utils/StorageSlot.sol";
import {Strings} from "../utils/Strings.sol";
import {Time} from "../utils/types/Time.sol";

contract Dummy1234 {}
2 changes: 1 addition & 1 deletion contracts/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"!/mocks/**/*"
],
"scripts": {
"prepare": "bash ../scripts/prepare-contracts-package.sh",
"prepack": "bash ../scripts/prepack.sh",
"prepare-docs": "cd ..; npm run prepare-docs"
},
"repository": {
Expand Down
2 changes: 2 additions & 0 deletions contracts/proxy/utils/UUPSUpgradeable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {ERC1967Utils} from "../ERC1967/ERC1967Utils.sol";
* `UUPSUpgradeable` with a custom implementation of upgrades.
*
* The {_authorizeUpgrade} function must be overridden to include access restriction to the upgrade mechanism.
*
* @custom:stateless
*/
abstract contract UUPSUpgradeable is IERC1822Proxiable {
/// @custom:oz-upgrades-unsafe-allow state-variable-immutable
Expand Down
2 changes: 2 additions & 0 deletions contracts/token/ERC1155/utils/ERC1155Holder.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import {IERC1155Receiver} from "../IERC1155Receiver.sol";
*
* IMPORTANT: When inheriting this contract, you must include a way to use the received tokens, otherwise they will be
* stuck.
*
* @custom:stateless
*/
abstract contract ERC1155Holder is ERC165, IERC1155Receiver {
/**
Expand Down
2 changes: 2 additions & 0 deletions contracts/token/ERC721/utils/ERC721Holder.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import {IERC721Receiver} from "../IERC721Receiver.sol";
* Accepts all token transfers.
* Make sure the contract is able to use its token with {IERC721-safeTransferFrom}, {IERC721-approve} or
* {IERC721-setApprovalForAll}.
*
* @custom:stateless
*/
abstract contract ERC721Holder is IERC721Receiver {
/**
Expand Down
2 changes: 2 additions & 0 deletions contracts/utils/Context.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ pragma solidity ^0.8.20;
* is concerned).
*
* This contract is only required for intermediate, library-like contracts.
*
* @custom:stateless
*/
abstract contract Context {
function _msgSender() internal view virtual returns (address) {
Expand Down
2 changes: 2 additions & 0 deletions contracts/utils/Multicall.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import {Address} from "./Address.sol";

/**
* @dev Provides a function to batch together multiple calls in a single external call.
*
* @custom:stateless
*/
abstract contract Multicall {
/**
Expand Down
2 changes: 2 additions & 0 deletions contracts/utils/introspection/ERC165.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import {IERC165} from "./IERC165.sol";
* return interfaceId == type(MyInterface).interfaceId || super.supportsInterface(interfaceId);
* }
* ```
*
* @custom:stateless
*/
abstract contract ERC165 is IERC165 {
/**
Expand Down
1 change: 1 addition & 0 deletions hardhat.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ module.exports = {
},
},
exposed: {
imports: true,
initializers: true,
exclude: ['vendor/**/*'],
},
Expand Down
97 changes: 92 additions & 5 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
"name": "openzeppelin-solidity",
"description": "Secure Smart Contract library for Solidity",
"version": "4.9.2",
"private": true,
"files": [
"/contracts/**/*.sol",
"/build/contracts/*.json",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure the build are getting in ?
The line above only gets solidity files

Copy link
Collaborator

Choose a reason for hiding this comment

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

actually, the entier "files" section of this package.json is probably useless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the root package.json, that we don't publish any more. This "files" field probably doesn't matter but I preferred keeping it and just removing the directory that will no longer be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My reasoning is that there may be some workflow that relies on npm pack at the root, I don't know.

"!/contracts/mocks/**/*"
],
"scripts": {
Expand All @@ -20,7 +20,6 @@
"lint:sol": "prettier --log-level warn --ignore-path .gitignore '{contracts,test}/**/*.sol' --check && solhint '{contracts,test}/**/*.sol'",
"lint:sol:fix": "prettier --log-level warn --ignore-path .gitignore '{contracts,test}/**/*.sol' --write",
"clean": "hardhat clean && rimraf build contracts/build",
"prepare": "scripts/prepare.sh",
"prepack": "scripts/prepack.sh",
"generate": "scripts/generate/run.js",
"release": "scripts/release/release.sh",
Expand Down Expand Up @@ -59,6 +58,7 @@
"@nomiclabs/hardhat-web3": "^2.0.0",
"@openzeppelin/docs-utils": "^0.1.4",
"@openzeppelin/test-helpers": "^0.5.13",
"@openzeppelin/upgrade-safe-transpiler": "^0.3.30",
"@openzeppelin/upgrades-core": "^1.20.6",
"array.prototype.at": "^1.1.1",
"chai": "^4.2.0",
Expand All @@ -70,7 +70,7 @@
"glob": "^10.3.5",
"graphlib": "^2.1.8",
"hardhat": "^2.9.1",
"hardhat-exposed": "^0.3.11",
"hardhat-exposed": "^0.3.12-1",
"hardhat-gas-reporter": "^1.0.4",
"hardhat-ignore-warnings": "^0.2.0",
"keccak256": "^1.0.2",
Expand Down
17 changes: 14 additions & 3 deletions scripts/prepack.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,20 @@ set -euo pipefail
shopt -s globstar

# cross platform `mkdir -p`
node -e 'fs.mkdirSync("build/contracts", { recursive: true })'
mkdirp() {
node -e "fs.mkdirSync('$1', { recursive: true })"
}

cp artifacts/contracts/**/*.json build/contracts
rm build/contracts/*.dbg.json
# cd to the root of the repo
cd "$(git rev-parse --show-toplevel)"

npm run clean

env COMPILE_MODE=production npm run compile

mkdirp contracts/build/contracts
cp artifacts/contracts/**/*.json contracts/build/contracts
rm contracts/build/contracts/*.dbg.json
node scripts/remove-ignored-artifacts.js

cp README.md contracts/
15 changes: 0 additions & 15 deletions scripts/prepare-contracts-package.sh

This file was deleted.

10 changes: 0 additions & 10 deletions scripts/prepare.sh

This file was deleted.

2 changes: 1 addition & 1 deletion scripts/remove-ignored-artifacts.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const ignorePatternsSubtrees = ignorePatterns
.concat(ignorePatterns.map(pat => path.join(pat, '**/*')))
.map(p => p.replace(/^\//, ''));

const artifactsDir = 'build/contracts';
const artifactsDir = 'contracts/build/contracts';
const buildinfo = 'artifacts/build-info';
const filenames = fs.readdirSync(buildinfo);

Expand Down
9 changes: 7 additions & 2 deletions scripts/upgradeable/transpile.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@

set -euo pipefail -x

VERSION="$(jq -r .version contracts/package.json)"
DIRNAME="$(dirname -- "${BASH_SOURCE[0]}")"

bash "$DIRNAME/patch-apply.sh"
sed -i "s/<package-version>/$VERSION/g" contracts/package.json
git add contracts/package.json

npm run clean
npm run compile
Expand All @@ -24,7 +27,8 @@ fi
# -p: emit public initializer
# -n: use namespaces
# -N: exclude from namespaces transformation
npx @openzeppelin/upgrade-safe-transpiler@latest -D \
# -q: partial transpilation using @openzeppelin/contracts as peer project
npx @openzeppelin/upgrade-safe-transpiler -D \
-b "$build_info" \
-i contracts/proxy/utils/Initializable.sol \
-x 'contracts-exposed/**/*' \
Expand All @@ -36,7 +40,8 @@ npx @openzeppelin/upgrade-safe-transpiler@latest -D \
-x '!contracts/proxy/beacon/IBeacon.sol' \
-p 'contracts/**/presets/**/*' \
-n \
-N 'contracts/mocks/**/*'
-N 'contracts/mocks/**/*' \
-q '@openzeppelin/'
Copy link
Contributor Author

@frangio frangio Sep 27, 2023

Choose a reason for hiding this comment

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

@Amxx Does this look right to you? We have to do this because the contract source paths already have a contracts/ prefix, but it's not really true that @openzeppelin/ is the peer dependency. Ideally I would write something like @openzeppelin/contracts/=contracts/, like a Foundry remapping, but I don't want to complicate the transpiler more...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I used to do -q '@openzeppelin/contracts/.. for my tests. What you wrote works just fine.

We can change to a remaping / remapings in a further version of the transpiler


# delete compilation artifacts of vanilla code
npm run clean
Loading
Loading