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

feat: update spec for v0.7.0 #32

Merged
merged 4 commits into from
Jan 24, 2024
Merged

Conversation

jaypaik
Copy link
Collaborator

@jaypaik jaypaik commented Jan 22, 2024

Various updates to the spec and reference implementation interfaces. This builds on the updates that were made in #20.

  • Added a note for canSpendNativeToken to clarify that the plugin can spend up to the amount that it sends to the account in the same call even if this value is false.
  • Changes related to feat!: disallow using dependencies for hooks #29.
    • PluginManifest's dependencyInterfaceIds comment no longer includes hooks.
    • installPlugin's comment on the dependencies param is now explicit about each FunctionReference being a validation function.
  • Renamed the callbacksSucceeded param of the PluginUninstalled event to onUninstallSucceeded, as there are no longer multiple callbacks for uninstalls after feat: [v0.7.0] Cut permitted call hooks & injection #20.
  • Renamed pluginInitData param of installPlugin to pluginInstallData to be more consistent with the pluginUninstallData param of uninstallPlugin.
  • FunctionReference type declaration has been moved from IAccountLoupe.sol to IPluginManager.sol, since the former is an optional interface.
  • installPlugin updates:
    • Included instruction to store whether the plugin can spend the account's native tokens, which was implied but missing.
    • Included instruction to store interface IDs for the manifest's interfaceIds, which was implied but missing.
    • Included the instruction to also emit dependencies in the PluginInstall event, which was implied but missing.
  • uninstallPlugin updates:
    • Removed reference to hook dependencies.
    • Included instruction to remove interface IDs, which was implied but missing.
    • Included note that if there are duplicate hooks or interface IDs that have been added by other plugins, they must persist until the last plugin is uninstalled.
    • Included instruction to emit the PluginUninstalled event, which was implied but missing.
  • Added clarity around how duplicate hooks should be handled during execution.
  • Consistency improvements across comments.
  • Fixed typos.

@jaypaik jaypaik requested a review from adam-alchemy January 22, 2024 07:25
@jaypaik jaypaik force-pushed the 01-22-feat_update_spec_for_v0.7.0 branch 2 times, most recently from 10b8de7 to 3cd3a0d Compare January 23, 2024 04:41
@jaypaik jaypaik force-pushed the 01-22-feat_update_spec_for_v0.7.0 branch from 3cd3a0d to 34fec34 Compare January 23, 2024 20:06
@jaypaik jaypaik force-pushed the 01-21-feat_run_duplicate_pre-hooks_just_once branch from 540df6f to f298cfe Compare January 23, 2024 20:07
@jaypaik jaypaik force-pushed the 01-22-feat_update_spec_for_v0.7.0 branch from 34fec34 to d6253cd Compare January 23, 2024 20:07
@jaypaik jaypaik force-pushed the 01-21-feat_run_duplicate_pre-hooks_just_once branch from f298cfe to 1869bd4 Compare January 23, 2024 23:46
@jaypaik jaypaik force-pushed the 01-22-feat_update_spec_for_v0.7.0 branch from d6253cd to 894d3e7 Compare January 23, 2024 23:47
Base automatically changed from 01-21-feat_run_duplicate_pre-hooks_just_once to main January 23, 2024 23:55
@jaypaik jaypaik force-pushed the 01-22-feat_update_spec_for_v0.7.0 branch from 894d3e7 to c5e9fb3 Compare January 23, 2024 23:56
Copy link
Contributor

@adam-alchemy adam-alchemy left a comment

Choose a reason for hiding this comment

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

Couple comments!

src/account/UpgradeableModularAccount.sol Show resolved Hide resolved
standard/ERCs/erc-6900.md Outdated Show resolved Hide resolved
@@ -87,7 +87,7 @@ Each step is modular, supporting different implementations for each execution fu

**Modular Smart Contract Accounts** **MUST** implement

- `IAccount` from [ERC-4337](./eip-4337.md).
- `IAccount.sol` from [ERC-4337](./eip-4337.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice for consistency here, but eventually we should find a way to make this less solidity-specific.

standard/ERCs/erc-6900.md Outdated Show resolved Hide resolved
standard/ERCs/erc-6900.md Outdated Show resolved Hide resolved
standard/ERCs/erc-6900.md Outdated Show resolved Hide resolved
@jaypaik jaypaik force-pushed the 01-22-feat_update_spec_for_v0.7.0 branch from c5e9fb3 to 2ba4978 Compare January 24, 2024 00:05
@jaypaik jaypaik requested a review from adam-alchemy January 24, 2024 14:31
@jaypaik jaypaik force-pushed the 01-22-feat_update_spec_for_v0.7.0 branch from 9cfac88 to 4338c4d Compare January 24, 2024 17:04
Copy link
Contributor

@adam-alchemy adam-alchemy left a comment

Choose a reason for hiding this comment

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

Looks good! One tiny nit:

standard/ERCs/erc-6900.md Outdated Show resolved Hide resolved
@jaypaik jaypaik merged commit 21fd644 into main Jan 24, 2024
3 checks passed
@jaypaik jaypaik deleted the 01-22-feat_update_spec_for_v0.7.0 branch January 24, 2024 22:33
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