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

Extract deployment transaction from upgraded contract #46

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

jagodarybacka
Copy link
Collaborator

@jagodarybacka jagodarybacka commented Jan 23, 2024

What

After the proxy contract was upgraded we had a problem with extracting the deployment transaction via contract.deploymentTransaction() from ethers.

It was happening because OpenZeppelin's upgradeProxy was replacing deployment tx on the proxy contract but the field was different than what ethers function expected. Let's make it work by directly using the field that upgradeProxy adds to the contact's object.

deployProxy implementation:

source
image

deploymentTransaction implementation:

source
image

After proxy contract is upgraded we had a problem with
extracting deployment transaction via
`contract.deploymentTransaction()` from ethers. It was happening
because OpenZeppelin's `upgradeProxy` was replacing deployment
tx on the proxy contract but the field was different than what ethers
function expected. Let's make it work by directly using the field
that `upgradeProxy` adds on the contact's object.
@jagodarybacka jagodarybacka self-assigned this Jan 23, 2024
@jagodarybacka
Copy link
Collaborator Author

jagodarybacka commented Jan 23, 2024

Other things I noticed that could be improved in hardhat-helpers are:

  • there are no unit tests for upgrades here - we are using both deployProxy and upgradeProxy in the projects where this package is used and there is a decent test coverage there but I think it would be nice to have unit tests coverage here as well
  • there is a problem with using typechain's contract interfaces like upgradeProxy<ContractTypeFromTypechain>() - the types don't match, would be nice to fix it at some point
    EDIT: found a PR that would fix that issue

Lmk what you think about the priorities of these issues, they aren't blocking but I think nice to have 😉

src/upgrades.ts Outdated
Comment on lines 156 to 160
// This is a workaround to get the deployment transaction. The upgradeProxy attaches
// the deployment transaction to the field under a different name than ethers
// contract.deploymentTransaction() function expects.
const deploymentTransaction =
newContractInstance.deployTransaction as unknown as ContractTransactionResponse
Copy link
Member

Choose a reason for hiding this comment

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

Does it mean it is an issue in the OpenZeppelin upgrade plugin? If so, we should open an issue in their GitHub repository and link it from here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably it is something on their side unless that's again an issue with changes between different major ethers versions. Will check what is going on, if ethers was ever using that field name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah so maybe someone forgot to fix that - in ethers v5 there is indeed deployTransaction (docs) but OpenZeppelin's hardhat-upgrades package is already on ethers v6 (link)

@jagodarybacka
Copy link
Collaborator Author

Found a PR that would fix it on the OpenZeppelin's side

@pdyraga
Copy link
Member

pdyraga commented Jan 23, 2024

Other things I noticed that could be improved in hardhat-helpers are:

  • there are no unit tests for upgrades here - we are using both deployProxy and upgradeProxy in the projects where this package is used and there is a decent test coverage there but I think it would be nice to have unit tests coverage here as well
  • there is a problem with using typechain's contract interfaces like upgradeProxy<ContractTypeFromTypechain>() - the types don't match, would be nice to fix it at some point
    EDIT: found a PR that would fix that issue

Lmk what you think about the priorities of these issues, they aren't blocking but I think nice to have 😉

Please open three separate issues in this repository: unit test for upgrades, types problem, and one to eliminate the workaround we implemented in this PR once it is fixed on the OpenZeppelin side. Let's please also post a link to the third issue from the workaround comment in the code.

The workaround itself looks good to me but I would like it to refer to the issue so that everyone working with this code is aware of the need to remove it at some point in future.

@jagodarybacka
Copy link
Collaborator Author

Issues added #47 #48 #49

@pdyraga
Copy link
Member

pdyraga commented Jan 24, 2024

Nice investigation! 🕵🏻‍♀️

@pdyraga pdyraga merged commit 92bd5a6 into main Jan 24, 2024
3 checks passed
@pdyraga pdyraga deleted the upgrade-proxy-issue branch January 24, 2024 08:47
@jagodarybacka jagodarybacka mentioned this pull request Jan 24, 2024
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

Successfully merging this pull request may close these issues.

2 participants