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

plugin-hardhat upgradeProxy upgradeTo call doesnt await receipt #354

Closed
jacobrosenthal opened this issue May 7, 2021 · 13 comments
Closed

Comments

@jacobrosenthal
Copy link

Weve largely had success with manual one off upgrades until when we started to script multiple upgrades at a time and kept getting

ProviderError: Transaction gas price supplied is too low. There is another transaction with same nonce in the queue. Try increasing the gas price or incrementing the nonce.

Adding an await receipt.wait(); after upgradeTo seemed to fix it for us
https://github.com/OpenZeppelin/openzeppelin-upgrades/blob/master/packages/plugin-hardhat/src/upgrade-proxy.ts#L37

But it seems like the codebase should be combed for other receipts it might be eating?

@frangio
Copy link
Contributor

frangio commented May 7, 2021

Thanks for reporting this issue!

We've purposely avoided unnecessarily waiting for transactions to mine, since it can introduce unwanted delays. In your case it seems like you want that delay, but the plugin doesn't give you any way to do that.

Something we could do is assign the upgrade transaction as the deployTransaction of the contract returned by upgradeProxy, similar to how we handle deployProxy.

inst.deployTransaction = deployTransaction;

If we did this you would be able to write

const v2 = await upgrades.upgradeProxy(proxyAddress, ImplV2);
await v2.deployed();

Would this work for you?

As a workaround, I think you should be able to use NonceManager to avoid resending a transaction with the same nonce, without the need to await in the middle.

@jacobrosenthal
Copy link
Author

That might work. As you say, we tried deployed() with upstream and I know it didnt work or didnt exist.

It seems like .deployed() isnt really used much in favor of .deployTransaction.wait() but if thats the way to smuggle out the receipt wed give it a shot

@frangio
Copy link
Contributor

frangio commented May 12, 2021

Fixed in 1c92fb7#diff-b28b1ed9821f38c64524ea13e19264bb0772f8376afffd5d8a1f8277621833eb and released in the latest version of the Hardhat plugin.

@jacobrosenthal Please try it out and let me know. Both .deployed() and .deployTransaction.wait() should work.

@5hanth
Copy link

5hanth commented Jul 3, 2021

@frangio curious if this can help my use-case. I want to deploy the new implementation and store that TX that will be used for a Governance voting eventually by _authorizeUpgrade(address newImplementation) to check if the given implementation has enough votes to go through. so far it appears to me that there isn't a way to make the deployment of v2 first and call upgrateTo(newImplementationAddress) at a later stage once the proposal is revealed. Please correct me if I'm missing anything here. I see there is a way to attach new implementation with the new factory but the tests do not cover examples to upgrade to new implementation after attach

const greeter3 = GreeterV3.attach(greeter3ImplAddr);

@Amxx
Copy link
Contributor

Amxx commented Jul 5, 2021

Hello @5hanth

You can use the prepareUpgrade function. This will check the compatibility of the new implementation and deploy it to chain but NOT make the upgrade.

This function will return the instance of the newly deployed version.

You can then do you governance operation and perform the upgrade at a later stage.

@5hanth
Copy link

5hanth commented Jul 6, 2021

@Amxx Yes, I was planning in that approach. But for some reason calling MyContractV1.upgradeTo(implAddress) fails with reason Address: low-level delegate call failed. Is this not the native way of upgrading after prepareUpgrade via plugin?

@frangio
Copy link
Contributor

frangio commented Jul 6, 2021

If you're using transparent proxies (the default) you need to call the upgrade function of the ProxyAdmin contract. You can find the admin address in your .openzeppelin/mainnet.json file (or other network).

@5hanth
Copy link

5hanth commented Jul 6, 2021

@frangio I'm using UUPS

@frangio
Copy link
Contributor

frangio commented Jul 6, 2021

@5hanth Can you post your question on the OpenZeppeiln Forum? Include more details about how you have defined _authorizeUpgrade.

@5hanth
Copy link

5hanth commented Jul 6, 2021

I posted on OpenZeppelin. Just in case anyone wants to know this:

function _authorizeUpgrade(address newImplementation)
        internal
        override
        view
    {
        require(
            hasRole(DEFAULT_ADMIN_ROLE, msg.sender));
        require(approvedProposalsExpiry[newImplementation] > block.timestamp);
    }

signer has an admin role. The error occurs on reading any state variable.

@joaquinperea
Copy link

Hi guys! I'm deploying a proxy contract and its implementation using "deployProxy" instruction but I get a "Timed out waiting for transaction" error. I use ".deployed()" to await transaction mined but is not working. Am I doing something wrong?

@frangio
Copy link
Contributor

frangio commented Nov 25, 2021

@joaquinperea You're not doing anything wrong. If it times out, you can retry the same deployProxy function call and it will resume waiting for the same transaction.

@zhibo-noumena
Copy link

Hello,

It seems now deployed() is deprecated, I am using await .waitForDeployment(), and it still returns before the contract getting upgraded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants