TokenScript icon indicating copy to clipboard operation
TokenScript copied to clipboard

draft new ERC for contract working with ts

Open SmartLayer opened this issue 3 years ago • 16 comments
trafficstars

@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.

SmartLayer avatar May 09 '22 21:05 SmartLayer

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

hboon avatar May 10 '22 04:05 hboon

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.

JamesSmartCell avatar May 16 '22 08:05 JamesSmartCell

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).

jot2re avatar May 16 '22 08:05 jot2re

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.

JamesSmartCell avatar May 16 '22 11:05 JamesSmartCell

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.

jot2re avatar May 16 '22 12:05 jot2re

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;
}

JamesSmartCell avatar May 17 '22 01:05 JamesSmartCell

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

jot2re avatar May 17 '22 07:05 jot2re

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

beauwilliams avatar May 19 '22 07:05 beauwilliams

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.

jot2re avatar May 19 '22 07:05 jot2re

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!

beauwilliams avatar May 19 '22 07:05 beauwilliams

@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.

SmartLayer avatar May 20 '22 06:05 SmartLayer

@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.

oleggrib avatar May 25 '22 10:05 oleggrib

@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?

jot2re avatar May 25 '22 10:05 jot2re

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;

SmartLayer avatar May 25 '22 11:05 SmartLayer

@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?

jot2re avatar Jun 30 '22 09:06 jot2re

@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.

JamesSmartCell avatar Aug 25 '22 07:08 JamesSmartCell