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

ARC0047: Logic Signature Templates #226

Merged
merged 20 commits into from
Nov 9, 2023

Conversation

joe-p
Copy link
Contributor

@joe-p joe-p commented Jul 17, 2023

Provides a standardized way to define logic signatures so wallets can safely inform users what they are signing

@D4Ocrypto
Copy link

This great ARC will open the doors to so many new UX improvements! Thank you Joe!

@SudoWeezy SudoWeezy changed the title ARCXXXX: Logic Signature Templates ARC0047: Logic Signature Templates Jul 17, 2023
@github-actions github-actions bot removed the s-draft label Jul 28, 2023
@joe-p
Copy link
Contributor Author

joe-p commented Jul 28, 2023

Before going to final we'll need to add a section for wallet communication (perhaps a new WC method?), but there doesn't seem to be any objections to what is currently in the ARC.

@joe-p
Copy link
Contributor Author

joe-p commented Aug 4, 2023

dApps would need to send wallets both the ARC47 JSON and the variable values. Could be done in a payload like the following that is then processed by the wallet (substitute variables in TEAL, compile, and then sign the program)

{
   "arc47":{
      "name":"Address Opt-In",
      "description":"This program allows a given address to opt in the signer to any asset provided it's approved by the associated application",
      "program":"I3ByYWdtYSB2ZXJzaW9uIDgKI2RlZmluZSBNYXN0ZXJBcHBDYWxsIGxvYWQgMAoKLy8gU2F2ZSBNYXN0ZXJBcHBDYWxsCnR4biBHcm91cEluZGV4CmludCAxCisKc3RvcmUgMAoKLy8gVmVyaWZ5IGFtb3VudCBpcyAwCnR4biBBc3NldEFtb3VudAppbnQgMAo9PQphc3NlcnQKCi8vIFZlcmlmeSBzZW5kZXIgPT0gcmVjZWl2ZXIKdHhuIEFzc2V0UmVjZWl2ZXIKdHhuIFNlbmRlcgo9PQphc3NlcnQKCi8vIFZlcmlmeSBmZWUgaXMgMCAoY292ZXJlZCBieSBzZW5kZXIpCnR4biBGZWUKaW50IDAKPT0KYXNzZXJ0CgovLyBWZXJpZnkgYXNzZXRDbG9zZVRvIGlzIG5vdCBzZXQKdHhuIEFzc2V0Q2xvc2VUbwpnbG9iYWwgWmVyb0FkZHJlc3MKPT0KYXNzZXJ0CgovLyBWZXJpZnkgY2FsbGVkIGF0b21pY2FsbHkgd2l0aCBtYXN0ZXIgYXBwCk1hc3RlckFwcENhbGwKZ3R4bnMgQXBwbGljYXRpb25JRAppbnQgVE1QTF9ERUxFR0FURURfT1BUSU5fQVBQX0lECj09CmFzc2VydAoKLy8gVmVyaWZ5IHRoZSBjb3JyZWN0IG1ldGhvZCBpcyBiZWluZyBjYWxsZWQKTWFzdGVyQXBwQ2FsbApndHhuc2EgQXBwbGljYXRpb25BcmdzIDAKbWV0aG9kICJhZGRyZXNzT3B0SW4ocGF5LGF4ZmVyKXZvaWQiCj09CmFzc2VydAoKLy8gVmVyaWZ5IHRoZSBzZW5kZXIgaXMgdGhlIGNvcnJlY3QgYWRkcmVzcwpNYXN0ZXJBcHBDYWxsCmd0eG5zIFNlbmRlcgphZGRyIFRNUExfQVVUSE9SSVpFRF9BRERSRVNTCj09",
      "variables":[
         {
            "variable":"TMPL_DELEGATED_OPTIN_APP_ID",
            "name":"Delegated opt-in app ID",
            "type":"application",
            "description":"The ID of the application that will be used for verifying opt ins"
         },
         {
            "variable":"TMPL_AUTHORIZED_ADDRESS",
            "name":"Aurhotized address",
            "type":"address",
            "description":"The address that will be allowed to opt the signer into assets provided it's approved by the associated application"
         }
      ]
   },
   "values":{
      "TMPL_DELEGATED_OPTIN_APP_ID":1337,
      "TMPL_AUTHORIZED_ADDRESS":"46XYR7OTRZXISI2TRSBDWPUVQT4ECBWNI7TFWPPS6EKAPJ7W5OBXSNG66M"
   }
}

| --- | ----------- |
| `name` | The name of the logic signature. **SHOULD** be short and descriptive |
| `description` | A description of what the logic signature does |
| `program` | base64 encoding of the TEAL program source
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to base64 encode teal programs. They are unicode and so is JSON.

ARCs/arc-0047.md Outdated

#### Types

Any valid ABI type encoded in base16 (without the leading `0x`) with the following exceptions:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the type being encoded in hex? It's human readable unicode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the current wording made it unclear that I was referencing the value rather than the type itself. See if b02fc9c makes more sense

ARCs/arc-0047.md Outdated Show resolved Hide resolved
ARCs/arc-0047.md Outdated Show resolved Hide resolved
ARCs/arc-0047.md Outdated Show resolved Hide resolved
ARCs/arc-0047.md Outdated Show resolved Hide resolved
ARCs/arc-0047.md Outdated Show resolved Hide resolved
ARCs/arc-0047.md Outdated Show resolved Hide resolved
Comment on lines +132 to +136
#pragma version 9
byte "Hello, TMPL_NAME"
```

This is not valid because `TMPL_NAME` is not the full immediate argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

This example drives home that it's a big ask for the wallet to parse TEAL. According to these rules, it needs to accept:
byte TMPL_NAME; int 7, byteblock "hello\" " TEMPL_NAME for example.

I think we need to spec out the exact local rules that would allow a program to accept. I admit that's not easy.

Perhaps we should restrict things much more strongly. Only things that can be be properly escaped by the wallet, and result in a single byte X or int X instruction, for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah didn't think about constant blocks. I think enforcing byte x/int x is reasonable.

I also didn't think about ;. How about enforcing that any usage of a variable must match this regex /^(byte|int) TMPL_/

Copy link
Contributor Author

@joe-p joe-p Aug 7, 2023

Choose a reason for hiding this comment

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

Or probably /^\s*(byte|int) TMPL_/ to account for indented lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd allow pushint and pushbyte as well. I think there are good reasons to use them in templates, so that the inserted bytes end up exactly where you expect them.

(int and byte do some funny business around optimization, and moving the constants into constant blocks.)

@emg110
Copy link
Contributor

emg110 commented Sep 30, 2023

ARC has become excellent, please merge it ASAP Joe

Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

Do you want to have any rules for the names of template arguments?

ARCs/arc-0047.md Outdated Show resolved Hide resolved
Comment on lines +132 to +136
#pragma version 9
byte "Hello, TMPL_NAME"
```

This is not valid because `TMPL_NAME` is not the full immediate argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd allow pushint and pushbyte as well. I think there are good reasons to use them in templates, so that the inserted bytes end up exactly where you expect them.

(int and byte do some funny business around optimization, and moving the constants into constant blocks.)

Comment on lines +1 to +2
# Based on https://raw.githubusercontent.com/github/gitignore/main/Node.gitignore

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a .gitignore in here?

joe-p and others added 2 commits October 2, 2023 09:19
Co-authored-by: John Jannotti <[email protected]>
@SudoWeezy SudoWeezy merged commit eb29488 into algorandfoundation:main Nov 9, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants