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

Failure during manifest setup #1612

Open
DimaStebaev opened this issue Oct 9, 2023 · 4 comments · Fixed by #1650
Open

Failure during manifest setup #1612

DimaStebaev opened this issue Oct 9, 2023 · 4 comments · Fixed by #1650
Assignees
Labels
bug Something isn't working

Comments

@DimaStebaev
Copy link
Contributor

The bug description
There are 3 places where asynchronous functions are called wrong

It causes wrong behavior when a manifest file is not present.

Besides the manifest filename is generated incorrectly for globally known networks such as mainnet, goerli, sepolia, et cetera.

const correctManifestPath = `.openzeppelin/unknown-${chainId}.json`;

Probably the manifestSetup function is relevant only for actions on schain but it is used in upgrade script for mainnet smart contracts.

await manifestSetup(pathToManifest);

To Reproduce
Run upgrade of IMA mainnet smart contracts

  1. npx hardhat run migrations/upgradeMainnet.ts --network custom

Expected behavior
Smart contracts are upgraded.

Screenshots

> npx hardhat run migrations/upgradeMainnet.ts --network custom
Current Manifest file detected - will use this one
Error: ENOENT: no such file or directory, access '.openzeppelin/unknown-5.json'

Desktop:

  • OS: Ubuntu 23.04
  • Browser: Firefox
  • Version: 118.0.1

Smartphone:

  • Device: Google Pixel 6 Pro
  • OS: Android 13
  • Browser: Chrome
  • Version: 117.0.5938.153

Additional context
The issue is present for IMA 2.0.0-beta.16.

@DmytroNazarenko DmytroNazarenko added the bug Something isn't working label Oct 9, 2023
@DmytroNazarenko DmytroNazarenko added this to the SKALE 2.3 milestone Oct 9, 2023
@DmytroNazarenko
Copy link
Contributor

Added to 2.3, as a fix is small and crucial for upgrading mainnet IMA

@PolinaKiporenko
Copy link

After discussion with D3, Stan - nice to have in 2.3, but could be postponed to 2.4 due to its not planned in 2.3 to upgrade IMA contract

@PolinaKiporenko
Copy link

ima 2.1.0-beta.2

@OleksanderSalamatov
Copy link

Test strategy:

  1. Check that if there is no manifest file, upgrade script will print that there is no manifest
  2. Check that upgrade script detect <network_name>.json files, and not only unknown-X.json

Versions:
IMA: 2.1.0-beta.2

Result:

  1. Script is telling that there is no ProxyAdmin address in manifest, not that there is no manifest
➜  proxy git:(2.1.0-beta.2) ✗ npx hardhat run migrations/upgradeMainnet.ts --network custom
Error: No ProxyAdmin was found in the network manifest at getManifestAdmin (/root/Workspace/IMA/proxy/node_modules/@openzeppelin/hardhat-upgrades/src/admin.ts:65:11)
  1. Upgrade script correctly detects networkID via file name
➜  proxy git:(2.1.0-beta.2) ✗ ll .openzeppelin 
total 512K
-rw-r--r-- 1 root root 430K Feb 23 17:55 mainnet.json
-rw-r--r-- 1 root root  704 Feb 20 18:11 project.json
-rw-r--r-- 1 root root  76K Feb 23 18:09 sepolia.json
➜  proxy git:(2.1.0-beta.2) ✗ npx hardhat run migrations/upgradeMainnet.ts --network custom
Will mark updated version as 2.1.0-beta.2
Prepare upgrade of MessageProxyForMainnet
...

Summary:
First case should be fixed further, second is good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Code Review
Development

Successfully merging a pull request may close this issue.

5 participants