-
Notifications
You must be signed in to change notification settings - Fork 399
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 ERC: Permissionless Script Registry #530
Conversation
✅ All reviewers have approved. |
ERCS/erc-XXXXscriptregistry.md
Outdated
@@ -0,0 +1,150 @@ | |||
--- |
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.
--- | |
--- | |
eip: 7738 |
Assigning next sequential EIP/ERC/RIP number.
Please also update the filename.
ERCS/erc-XXXXscriptregistry.md
Outdated
title: Permissionless Script Registry | ||
description: Permissionless registry to fetch executable scripts for contracts | ||
author: Victor Zhang (@zhangzhongnan928), James Brown (@JamesSmartCell) | ||
discussions-to: https://ethereum-magicians.org/t/eip-permissionless-registry |
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.
discussions-to: https://ethereum-magicians.org/t/eip-permissionless-registry | |
discussions-to: https://ethereum-magicians.org/t/erc-7738-permissionless-script-registry/20503 |
I don't know how to include assets: doesn't matter if they are linked to /assets/eip-7738/ or /assets/erc-7738 they both fail. Seems that other ERCs link to "assets/eip-xxxx/" so this PR follows them. |
@JamesSmartCell could you also include an interface id? |
ERCS/erc-7738.md
Outdated
|
||
The scripts provided could be authenticated in various ways: | ||
|
||
1. The target contract which the setter specifies implements the `Ownable` interface. Once the script is fetched, the signature can be verified to match the Owner(). In the case of TokenScript this can be checked by a dapp or wallet using the TokenScript SDK, the TokenScript online verification service, or by extracting the signature from the XML, taking a keccak256 of the script and ecrecover the signing key address. |
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.
Do you mean to use ERC-173 ownable here? If it's ERC-173 please mention it and add to requirement
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.
Also this should be add to the specification
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.
Thankyou for the clarification. Added
ERCS/erc-7738.md
Outdated
2. If the contract does not implement Ownable, further steps can be taken: | ||
a. The hosting app/wallet can acertain the deployment key using 3rd party API or block explorer. The implementing wallet, dapp or viewer would then check the signature matches this deployment key. | ||
b. Signing keys could be pre-authenticated by a hosting app, using an embedded keychain. | ||
c. A governance token could allow a script council to authenticate requests to set and validate keys. |
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.
Are these a, b, c examples or actual requirements / recommendations?
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.
Yes, these are simply methods to authenticate that could be used if this is required by the implementor. So they belong in the reference not in the Spec or Overview.
|
||
/// @notice Get the scriptURI for the contract | ||
/// @return The scriptURI | ||
function scriptURI(address contractAddress) external view returns (string[] memory); |
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 are from ERC-5169 right? if so it should inherit the ERC-5169 interface instead
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.
ERC-5169 in entirety can't be applied here because although this interface follows the intent of ERC-5169, the functions in this interface have a different form; they require a contract address in addition to each method.
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.
Got it
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.
Left a few editorial changes requested
Hi |
The contract may not need this as it wouldn't be a discoverable token, more like a singleton similar to the ENS registry. However if required one can be added. |
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.
LGTM as draft. I will leave my personal peer tech review comments on ethereum-magicians in a follow up.
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.
All Reviewers Have Approved; Performing Automatic Merge...
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.
All Reviewers Have Approved; Performing Automatic Merge...
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.
All Reviewers Have Approved; Performing Automatic Merge...
Head branch was pushed to by a user without write access
6b708f7
The commit 1734c4a (as a parent of af59c65) contains errors. |
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.
All Reviewers Have Approved; Performing Automatic Merge...
This ERC proposes a permissionless registry to allow fetching of executable scripts for contracts.