-
Notifications
You must be signed in to change notification settings - Fork 1
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
Delegated Opt In Review #1
Open
joe-p
wants to merge
24
commits into
review
Choose a base branch
from
review_files
base: review
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
fa5b4e0
add back files for review
joe-p ce6d6b7
Fix typos from review
joe-p ee9ab06
update addr offset
joe-p 80cd20e
_ -> invalid_arg_count
joe-p 6f4f322
dont hash or prefix sig key for discoverability
joe-p 76ca460
more lsig checks, update tests
joe-p d4b111a
initi review updates
joe-p 978b12f
update TEALScript
joe-p 0dadbfe
doesn't allow opt-ins test
joe-p 9169a14
rm authAddr resolution
joe-p 962de7a
more validation in lsig
joe-p 76e7d34
use >= for mbr payment amount
joe-p f45b476
rm asset MBR from app
joe-p 3bb98bd
Update README.md
joe-p c5c2d95
use "global CurrentApplicationAddress"
joe-p b69d790
update README
joe-p 89e1515
unsetSignature -> revokeSignature
joe-p 7a559c5
update revokeSignature desc
joe-p c5d7203
update TEALScript
joe-p 4701fd7
verify txns in app call
joe-p 75ceab2
move app verificaiton to the top
joe-p 56c7aa0
fix typo
joe-p 5eb33dd
update README
joe-p bf072e6
use verifyTxn for boxMBRPayment
joe-p File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
## Abstract | ||
This ARC contains is an implementation of [ARCX](https://github.com/algorandfoundation/ARCs/pull/229). The goal is to provide users a way to delegate asset opt-ins while having a high degree of control compared to a standalone logic signature without an application. The expectation is that a single instance of this ARC will be deployed on Algorand networks. | ||
|
||
## Motivation | ||
[ARCX](https://github.com/algorandfoundation/ARCs/pull/229) provides a standard for delegated asset opt-ins, but there are some UX problems that need to be addressed. First, there needs to be a way to control whether the lsig is still functional or not. Without an application to control this, signing the logic signature program would be irreversible without rekeying. There also needs to be a standardized way to store and read signatures for a given account so dApps and other users can take advantaged of delegated opt-ins. | ||
|
||
## Specification | ||
This is an implementation of [ARCX](https://github.com/algorandfoundation/ARCs/pull/229). There is additional functionality provided by the application in this ARC in the methods of the application. | ||
|
||
For all methods, refer to the [ABI JSON description](./contracts/artifacts/DelegatedOptIn.abi.json) for complete descriptions of arguments. | ||
|
||
### Signature Storage | ||
|
||
`setSignature(byte[64],pay)void` is a method that allows a user to upload their signature to box storage | ||
|
||
|
||
### Opt-In | ||
|
||
`delegatedOptIn(pay,axfer)void` is an implementation of the [ARCX](https://github.com/algorandfoundation/ARCs/pull/229) interfaces. It verifies the MBR payment is sent to the account opting in and that it covers the ASA minimum balance requirement. | ||
|
||
### Storage | ||
|
||
| Type | Key | Description | | ||
| ---- | --- | ----------- | | ||
| Box | `auth-addr \|\| address` | Mapping of signer to lsig signature | | ||
|
||
## Rationale | ||
Box storage is used to store signatures to make them easily accessible for any user or app wishing to use them and act as a way to signify if delegated opt-ins should be allowed. | ||
|
||
## Backwards Compatibility | ||
N/A | ||
|
||
## Test Cases | ||
Tests written with Jest and algokit for the contract and logic signatures can be seen in [test.ts](./tests/test.ts). | ||
|
||
``` | ||
Delegated Opt In App | ||
create | ||
✓ creates the app (765 ms) | ||
setSignature | ||
✓ works with valid signature and lsig (790 ms) | ||
delegatedOptIn | ||
✓ works with valid lsig and method call (1453 ms) | ||
revokeSignature | ||
✓ sends back MBR (753 ms) | ||
✓ doesn't allow opt-ins (1186 ms) | ||
``` | ||
|
||
## Reference Implementation | ||
[delegated_optin_app.algo.ts](./contracts/delegated_optin_app.algo.ts) is the application written in TEALScript. | ||
|
||
[DelegatedOptIn.approval.teal](./contracts/artifacts/DelegatedOptIn.approval.teal) is the TEAL compiled from TEALScript. | ||
|
||
## Security Considerations | ||
|
||
The test cases test proper functionality of all methods, but there has been no extended effort in attempt to break the contract. Most of the functionality in the app and logic signatures is relatively simple, so the chances of unexpected bugs is relatively low. | ||
|
||
It should be made clear that signatures are stored mapped to the signer (`auth-addr || address`). This means when a signature is made known, every account with the signing account as the `auth-addr` can now be opted-in to assets via the delegated logic signature. | ||
|
||
## Copyright | ||
Copyright and related rights waived via <a href="https://creativecommons.org/publicdomain/zero/1.0/">CCO</a>. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
{ | ||
"name": "DelegatedOptIn", | ||
"desc": "", | ||
"methods": [ | ||
{ | ||
"name": "setSignature", | ||
"args": [ | ||
{ | ||
"name": "sig", | ||
"type": "byte[64]", | ||
"desc": "The signature of the lsig" | ||
}, | ||
{ | ||
"name": "boxMBRPayment", | ||
"type": "pay", | ||
"desc": "Payment to cover the contract MBR for box creation" | ||
} | ||
], | ||
"desc": "Set the signature of the lsig for the given account", | ||
"returns": { | ||
"type": "void", | ||
"desc": "" | ||
} | ||
}, | ||
{ | ||
"name": "delegatedOptIn", | ||
"args": [ | ||
{ | ||
"name": "mbrPayment", | ||
"type": "pay", | ||
"desc": "Payment to the receiver that covers the ASA MBR" | ||
}, | ||
{ | ||
"name": "optIn", | ||
"type": "axfer", | ||
"desc": "The opt in transaction, presumably from the open opt-in lsig" | ||
} | ||
], | ||
"desc": "Verifies that the opt in is allowed", | ||
"returns": { | ||
"type": "void", | ||
"desc": "" | ||
} | ||
}, | ||
{ | ||
"name": "revokeSignature", | ||
"args": [], | ||
"desc": "Delete the signature from box storage. This will disable delegated opt-ins andreturn the box MBR balance. This app call should include an extra 0.001 ALGO to coverthe inner transaction fee for the payment.", | ||
"returns": { | ||
"type": "void", | ||
"desc": "" | ||
} | ||
} | ||
] | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I can see why this is a nice, useful app to use in combination with the ARC-50, but I don't see why it should be considered "special". The point of ARC-50 was to give users control - let them have an app that they trust to check opt-ins for them. This is one of them, but if it's the only one, then the flexibility that ARC-50 promises is reduced to the flexibility that this particular implementation provides.
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.
Yeah the idea isn't that this is the only ARC50 implementation, but it's a standardized one that every wallet/dApp in the ecosystem can integrate. Other people can make whatever implementations they want, but the more implementations there are the more fragmented the ecosystem can get.