-
Notifications
You must be signed in to change notification settings - Fork 287
Support PolkaVM for Hardhat Upgrades Plugin #1190
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
base: master
Are you sure you want to change the base?
Conversation
|
Caution Review the following alerts detected in dependencies. According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
This compiles the core package for both EVM and PVM, but I don't see a way for it to actually run any test cases with the PVM code path.
We should try to find a way to run all of the tests (in both core and plugin-hardhat) using the PVM code path, if feasible, perhaps using a custom GitHub workflow to modify the way it runs. Even if that is not feasible, we should aim to add at least one mainline test scenario (e.g. deploy and upgrade a proxy) using PVM.
(If that has been tested manually already, I think this would be ok as-is, and the automated tests can be added later)
|
|
||
| const { ethers, upgrades } = require('hardhat'); | ||
|
|
||
| describe('happy path', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test file name doesn't appear in the results when not using ava, so we could add more context in the test name.
| describe('happy path', async () => { | |
| describe('beacon happy path', async () => { |
|
|
||
| const { ethers, upgrades } = require('hardhat'); | ||
|
|
||
| describe('happy path', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| describe('happy path', async () => { | |
| describe('transparent happy path', async () => { |
| const { expect } = require('chai'); | ||
| const { ethers, upgrades } = require('hardhat'); | ||
|
|
||
| describe('happy path', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| describe('happy path', async () => { | |
| describe('uups happy path', async () => { |
| // test.before(async t => { | ||
| // t.context.Greeter = await ethers.getContractFactory('Greeter'); | ||
| // t.context.GreeterV2 = await ethers.getContractFactory('GreeterV2'); | ||
| // t.context.GreeterV3 = await ethers.getContractFactory('GreeterV3'); | ||
| // }); | ||
|
|
||
| // test('happy path', async t => { | ||
| // const { Greeter, GreeterV2, GreeterV3 } = t.context; | ||
|
|
||
| // const greeter = await upgrades.deployProxy(Greeter, ['Hello, Hardhat!'], { kind: 'transparent' }); | ||
|
|
||
| // const greeter2 = await upgrades.upgradeProxy(greeter, GreeterV2); | ||
| // await greeter2.waitForDeployment(); | ||
| // await greeter2.resetGreeting(); | ||
|
|
||
| // const greeter3ImplAddr = await upgrades.prepareUpgrade(await greeter.getAddress(), GreeterV3); | ||
| // const greeter3 = GreeterV3.attach(greeter3ImplAddr); | ||
| // const version3 = await greeter3.version(); | ||
| // t.is(version3, 'V3'); | ||
| // }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the commented parts needed?
| // test.before(async t => { | |
| // t.context.Greeter = await ethers.getContractFactory('Greeter'); | |
| // t.context.GreeterV2 = await ethers.getContractFactory('GreeterV2'); | |
| // t.context.GreeterV3 = await ethers.getContractFactory('GreeterV3'); | |
| // }); | |
| // test('happy path', async t => { | |
| // const { Greeter, GreeterV2, GreeterV3 } = t.context; | |
| // const greeter = await upgrades.deployProxy(Greeter, ['Hello, Hardhat!'], { kind: 'transparent' }); | |
| // const greeter2 = await upgrades.upgradeProxy(greeter, GreeterV2); | |
| // await greeter2.waitForDeployment(); | |
| // await greeter2.resetGreeting(); | |
| // const greeter3ImplAddr = await upgrades.prepareUpgrade(await greeter.getAddress(), GreeterV3); | |
| // const greeter3 = GreeterV3.attach(greeter3ImplAddr); | |
| // const version3 = await greeter3.version(); | |
| // t.is(version3, 'V3'); | |
| // }); |
|
Can we also add some simple negative tests? These could be based on
|
| it('invalid upgrade', async () => { | ||
| const { Greeter, Invalid } = context; | ||
| const greeter = await upgrades.deployProxy(Greeter, ['Hola mundo!'], { kind: 'uups' }); | ||
| await expect(upgrades.upgradeProxy(greeter, Invalid)).to.be.rejectedWith(/New storage layout is incompatible.*/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case was originally intended to check for unsafe patterns in the implementation contract itself, unrelated to storage layout. Since there is no selfdestruct, we should test for another pattern such as with a constructor in InvalidPVMProxiable.
Specifically, I would suggest:
- Add a storage variable
string greeting;inInvalidPVMProxiableso that its storage layout matches that ofGreeterProxiable - Add a constructor in
InvalidPVMProxiable - Change this
expectto look for an error about the constructor.
Support precompiled contracts for PolkaVM deployments.
What should be done to move this PR to ready state:
resolcNB: NodeJS version was pushed to 22.x, because Hardhat Polkadot plugin requires built-in WebSocket, that is present only in v22 and higher