-
Notifications
You must be signed in to change notification settings - Fork 167
Update account generation to use code from 5.4.0-rc.1 #577
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
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.
Thanks for the PR. Here is a checklist of some remaining items that are needed:
- The minimum version should be updated to
^5.4.0
inpackages/core/solidity/src/utils/version.ts
- Please add a changeset, mentioning that this is a potentially breaking change because it requires Contracts v5.4.0.
- An Upgradeability section should be added to the Account tab in the UI, in
packages/ui/src/solidity/AccountControls.svelte
- Should the Experimental status be removed from the Account tab in the UI? If so, the following need to be updated:
experimentalContracts={['Stablecoin', 'RealWorldAsset', 'Account']} <button class:selected={tab === 'Account'} on:click={() => (tab = 'Account')}> Account* </button> contracts-wizard/packages/ui/src/solidity/AccountControls.svelte
Lines 27 to 30 in df1bdff
<div class="text-sm text-gray-500"> <strong>* Experimental:</strong> <span class="italic">Some of the following features are not audited and are subject to change</span> </div> - The note in https://github.com/OpenZeppelin/contracts-wizard/blob/update/account-from-vanilla-5.4.0-rc/packages/core/solidity/README.md#contract-types
- Evaluate or test whether this works with Remix, downloaded Hardhat package, or downloaded Foundry package. If it should work, update the conditions in
contracts-wizard/packages/ui/src/solidity/App.svelte
Lines 150 to 167 in df1bdff
} else if ( opts?.kind === 'Stablecoin' || opts?.kind === 'RealWorldAsset' || opts?.kind === 'Account' || (opts?.kind === 'ERC20' && opts.crossChainBridging) ) { return { openInRemix: false, downloadHardhat: false, downloadFoundry: false, }; } else { return { openInRemix: true, downloadHardhat: true, downloadFoundry: true, }; } - Schemas/definitions for function calling from AI assistants/MCP should be updated to allow upgradeability for Account. This includes:
contracts-wizard/packages/ui/api/ai-assistant/function-definitions/solidity.ts
Lines 235 to 239 in df1bdff
upgradeable: { type: 'boolean', enum: [false], description: 'Upgradeability is not yet available for features that use @openzeppelin/community-contracts', }, packages/mcp/src/solidity/schemas.ts
return; | ||
} | ||
case 'ERC7702': { | ||
// Add locking constructor |
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.
If upgradeable is false, none of the imports use Initializable. In that case, does this still need to disable initializers? If so, it should add the import:
// Add locking constructor | |
// Add locking constructor | |
c.addParent({ | |
name: 'Initializable', | |
path: '@openzeppelin/contracts/proxy/utils/Initializable.sol', | |
}); |
name: 'ERC7739', | ||
path: '@openzeppelin/contracts/utils/cryptography/signers/draft-ERC7739.sol', | ||
}); | ||
} else if (opts.signatureValidation === 'ERC1271' && !opts.signatureValidation) { |
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 condition is not possible.
c.addNatspecTag('@custom:oz-upgrades-unsafe-allow', 'constructor'); | ||
c.addConstructorCode(`_disableInitializers();`); | ||
|
||
// todo: error if upgradeable is set ? |
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.
If this should be an error, it can throw OptionsError
and the UI can catch and display it. See
throw new OptionsError({ |
<input bind:value={opts.proposalThreshold} placeholder="0" use:error={errors?.proposalThreshold} /> |
"@openzeppelin/contracts": "^5.4.0-rc.1", | ||
"@openzeppelin/contracts-upgradeable": "^5.4.0-rc.1", |
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.
These should be bumped to 5.4.0 when it is released, before merging this PR.
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.
|
Upgradeable version is a bit challenging because only part of the parent contracts must be used in their upgradeable version.
Only contract that should be used from the upgradeable packages are:
while
EIP712
, andERC7739
have an upgradeable version, they should not be used here. Using the vanila version ofEIP712
so that the domain doesn't have to be initialized, and is shared accross all clones/proxies.