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

Add txOverrides option for overriding transaction parameters #852

Merged
merged 20 commits into from
Aug 3, 2023

Conversation

passabilities
Copy link
Contributor

@passabilities passabilities commented Jul 26, 2023

  • allows tx overrides object to be passed into the options parameter to use when submitting a tx

Fixes #85

@ericglau
Copy link
Member

ericglau commented Jul 29, 2023

Thanks for this PR! This looks like a reasonable approach which would address #85. Particularly, since deploy.ts in both Hardhat and Truffle are used for all deployments (including proxies, proxy-related contracts and implementations), setting the overrides there would cover the various deployment transactions.

I’ve made some changes to separate the option type for Hardhat and Truffle, since Truffle does not use ethers.js.
Also removed the .idea folder since that is environment-specific and would be more suitable in a local configuration.

Additional changes:

  • The Truffle option still needs a more specific type. Truffle is untyped so this will require adding an interface similar to what is done in truffle.ts.
  • Any transactions other than deployments (for example, upgrading a proxy) also need to respect the new options.
    • Hardhat
    • Truffle
  • Add tests.
    • Hardhat
    • Truffle
  • Add documentation.
  • Add changelog entries.

@ericglau ericglau marked this pull request as draft July 29, 2023 02:04
@ericglau ericglau changed the title Allow Optional TX Override Params Add option for overriding transaction parameters Jul 31, 2023
@ericglau ericglau changed the title Add option for overriding transaction parameters Add txOverrides option for overriding transaction parameters Jul 31, 2023
@ericglau ericglau marked this pull request as ready for review July 31, 2023 21:20
@ericglau ericglau requested a review from frangio July 31, 2023 21:21
Comment on lines +31 to +32
const overrides = opts.txOverrides ? [opts.txOverrides] : [];
const upgradeTx = await beaconContract.upgradeTo(nextImpl, ...overrides);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you just write upgradeTo(nextImpl, overrides)?

Copy link
Member

Choose a reason for hiding this comment

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

That would cause an error because then overrides appears as an array and Ethers tries to use it as part of the actual function arguments.

We need to spread the array because its element opts.txOverrides is the actual overrides object, and if that is not present, then it will be excluded.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry my question was actually if we write upgradeTo(nextImpl, opts.txOverrides) 🙂

Copy link
Member

@ericglau ericglau Aug 3, 2023

Choose a reason for hiding this comment

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

That doesn't work, but we could use either:
upgradeTo(nextImpl, opts.txOverrides ?? {})
or
upgradeTo(nextImpl, withDefaults(opts).txOverrides)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that these options will work with Truffle, I recall it working weird if you pass an empty object.

Copy link
Member

Choose a reason for hiding this comment

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

Right, these don't work for Truffle. Will leave them as-is for both Hardhat and Truffle, for consistency.

@ericglau ericglau merged commit 40b2d07 into OpenZeppelin:master Aug 3, 2023
9 checks passed
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.

Override transaction options (from, gas, gasPrice) in create and upgrade proxy
3 participants