SmartTokenLabs / TokenScript

TokenScript schema, specs and paper
http://tokenscript.org
MIT License
242 stars 71 forks source link

draft new ERC for contract working with ts #430

Open SmartLayer opened 2 years ago

SmartLayer commented 2 years ago

@JamesSmartCell this replaces https://github.com/TokenScript/ScriptURISubmission @jot2re I deleted the https://github.com/TokenScript/ScriptURISubmission and this one you have permission to write.

hboon commented 2 years ago

I don't have commit rights. Pushed 2 commits to https://github.com/hboon/TokenScript/commits/new_ercs

JamesSmartCell commented 2 years ago
struct ScriptURI {
    ScriptURIElement[] scriptURIElements;
}
struct ScriptURIElement {
    string URIOfScript;
    string URIOfSignature;
}

This should probably be:

struct ScriptURI {
    ScriptURIElement[] scriptURIElements;
}
struct ScriptURIElement {
    string scriptURI;
    string URIOfSignature; // <--- why is this a URI
    //bytes scriptSignature; // <--- possibly bytes signature? This will be smaller
}

Why would you have a URI to a signature when you could include the signature itself, which will often be shorter or the same length. Is it because we need to include the type of signature? In which case, it makes sense.

jot2re commented 2 years ago

Why would you have a URI to a signature when you could include the signature itself, which will often be shorter or the same length. Is it because we need to include the type of signature? In which case, it makes sense.

If we don't have an URI for the signature then every update of the script will have to involve writing to the blockchain. If we simply use an URI to the signature then the scriptPrivKey can be used without blockchain interaction to sign updated scripts (assuming the content of the URI can be updated, like on a webserver).

JamesSmartCell commented 2 years ago

Why would you have a URI to a signature when you could include the signature itself, which will often be shorter or the same length. Is it because we need to include the type of signature? In which case, it makes sense.

If we don't have an URI for the signature then every update of the script will have to involve writing to the blockchain. If we simply use an URI to the signature then the scriptPrivKey can be used without blockchain interaction to sign updated scripts (assuming the content of the URI can be updated, like on a webserver).

It doesn't seem secure that way - but admittedly it is more convenient.

jot2re commented 2 years ago

The idea of this approach is to allow the issuer to use a less critical key (then the smart contract key) to make updates to the script.

JamesSmartCell commented 2 years ago

The idea of this approach is to allow the issuer to use a less critical key (then the smart contract key) to make updates to the script.

Ok I see; because the Verification Key addresshash is provided separately in the contract it secures the remote signature.

In that case then my only remaining request for change is a small pedantic one; that is I don't think the variable names should start with capitals, maybe:

struct ScriptURI {
    ScriptURIElement[] scriptURIElements;
}
struct ScriptURIElement {
    string scriptURI;
    string signatureURI;
}
jot2re commented 2 years ago

Ah, right. Sorry, I am Java damaged where object types are written with capitals 😂 I'll change it.

beauwilliams commented 2 years ago

Not sure if relevant as it appears you are planning to use URI and not upload key to contract directly, but storing keys in contracts is not secure as they can be retrieved. See article below for example https://medium.com/coinmonks/a-quick-guide-to-hack-private-variables-in-solidity-b45d5acb89c0

jot2re commented 2 years ago

We are planning on storing the public key of a public/private signing key pair. The public key is safe for anyone to know. In fact our proposal need it and it is this key that allows anyone to validate a signature on a script stored off-chain.

beauwilliams commented 2 years ago

We are planning on storing the public key of a public/private signing key pair. The public key is safe for anyone to know. In fact our proposal need it and it is this key that allows anyone to validate a signature on a script stored off-chain.

Ah perfect, yes that makes sense!

SmartLayer commented 2 years ago

@TokenScript/library I think you are moving the section out to xxx3 ERC, so when you do that, make sure it's always mentioned the public key to avoid ambigiouty.

oleggrib commented 2 years ago

@weiwu-zhang , @jot2re I have few comments for the ERCs

  1. for ERC5XX0 - better work with string[] , not struct and have option to upload/replace/add every single script or all-in-one-request. Besause in currebt structure if we have defined 100 scripts and admin wants to remove first script then all scripts will be shifted left and in blockchaine storage lot of updates can appear, and it can be very GAS-expencive. If we can remove single script then we can move last script to the slot of removed script and it can be much cheaper.

  2. better have "smart contract deployment key" as fallback option, but add variable "scriptSigner", to have option to transfer right to sign scripts/certficates to another wallet.

jot2re commented 2 years ago

@weiwu-zhang , @jot2re I have few comments for the ERCs

  1. for ERC5XX0 - better work with string[] , not struct and have option to upload/replace/add every single script or all-in-one-request. Besause in currebt structure if we have defined 100 scripts and admin wants to remove first script then all scripts will be shifted left and in blockchaine storage lot of updates can appear, and it can be very GAS-expencive. If we can remove single script then we can move last script to the slot of removed script and it can be much cheaper.
  2. better have "smart contract deployment key" as fallback option, but add variable "scriptSigner", to have option to transfer right to sign scripts/certficates to another wallet.

Regarding 1. I actually deliberately didn't specify this flexibility since how I understood @weiwu-zhang an ERC like this should only specify the smallest amount of constraints to make things work (since apparently ERC implementor very quickly lose attention 😅). @weiwu-zhang what do you think? Should we update the ERC to allow for the more flexibility as @oleggrib suggest? (BTW I would agree with Oleg, that this provides the most complete standard). In any case, I will remove the struct and let is just be string[].

Regarding 2. I am not sure I completely understand what you suggest. You mean adding the option of allowing another key to control the smart contract? I think this is a bit out of the scope of ERC5XX0, but instead handled by ERC5XX1. Or maybe I am misunderstanding?

SmartLayer commented 2 years ago

Options (I'd go for 2 and find an ERC to refer to, or an openzepplin convention).

  1. Deployment key issues certificates, whose subject is the script signing key;
  2. Admin key issues certificates, whose subject is the script signing key; (practical reason: deployment is done by a different team specialised at deployment and has no authority);
  3. Admin key updates smart contract which allows a new script signing key issued; no certificates;
  4. Admin key updates smart contract returns new certificate issuing key, which issues certificates whose subject is the script signing key;
jot2re commented 2 years ago

@JamesSmartCell @weiwu-zhang what is the status on ERC 5XX0 and 5XX1? Are we continuing to keep this branch open and working on this until they are either accepted or rejected?

JamesSmartCell commented 1 year ago

@jot2re Yes it's still open

EIP-5169: https://github.com/ethereum/EIPs/pull/5169 EIP-5170: https://github.com/ethereum/EIPs/pull/5170

It looks like 5169 is pretty much in order now, after a bit of to-and-fro, once (if) it is accepted then 5170 would follow, since it depends on 5169 to make sense.