-
Notifications
You must be signed in to change notification settings - Fork 118
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
ARC-57: Application Action Routing #264
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: nullun <[email protected]>
ARCs/arc-draft_action_routing.md
Outdated
err | ||
``` | ||
|
||
For every action (combination of `ApplicationID === 0` and `OnCompletion`) there is a label that corresponds to that action. If that action is not implemented in the contract, its label should be replaced by a label that simply contains `err`. In the above TEAL, this is the `NOT_IMPLEMENTED` label. |
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.
An offchain parser has to check the destination of the label, that it contains err
only? This is going to lead to extreme fragility in that we now have to worry about badly written parsers of TEAL code. If we change the disassembly output slightly (for example, add comments to a line, as we sometimes do), will they break? If not, they must be implementing a fully correct TEAL parser?
Since you're asking them to skip constant blocks, they have to handle all possible ways to write constants. There's base32 and base64 strings, for example.
This entry point is certainly one way to do things, and I know TealScript does. But nothing else does, and it doesn't seem best for immutable contracts.
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.
An offchain parser has to check the destination of the label, that it contains err only? This is going to lead to extreme fragility in that we now have to worry about badly written parsers of TEAL code. If we change the disassembly output slightly (for example, add comments to a line, as we sometimes do), will they break? If not, they must be implementing a fully correct TEAL parser?
As long as nothing is added before the opcodes then parses shouldn't have a problem if they check to ensure only the beginning matches. I can update the ARC to be more clear about this.
Since you're asking them to skip constant blocks, they have to handle all possible ways to write constants. There's base32 and base64 strings, for example.
I'm not sure I follow. If parsers ignore bytecblock
and intcblock
lines there aren't other types of constant blocks, are there? I know there are pseudo-ops for different encodings, but aren't those ultimately compiled to either pushbytes
or put in the bytecblock
?
This entry point is certainly one way to do things, and I know TealScript does. But nothing else does, and it doesn't seem best for immutable contracts.
A side benefit of this approach is that you can check what other "actions" are supported, but I don't think that's very important.
The alternative, as you suggested on discord would be asserts in the beginning. Parsers would check for txn OnComplete; pushint DeleteApplication; !=; assert
or txn OnComplete; dup; pushint DeleteApplication; !=; assert; pushint UpdateApplication; != assert
(I think checking for update is just as important as being able to check for delete). The main downside of this approach is extra ops/bytes in the program, but it's not a lot.
What's the motivation behind this? The stated motivation doesn't explain what problem is being solved or goal is being achieved. |
Just updated the motivation and rationale sections |
Co-authored-by: nullun <[email protected]>
This ARC provides a standard way to handle an application call depending on the combination of the `OnCompletion` and `ApplicationID`. | ||
|
||
## Motivation | ||
App mutability is an important factor in a rational actors decision making when it comes to interacting with an app and sending funds to a contract account. If a contract is mutable, it is possible that funds could be stolen or lost after the initial program is changed or deleted. Currently, there is no easy way for an end-user to determine the mutability of a contract. It is possible to use static analysis on the decompiled TEAL, but this is not easily accessible and unrealistic for wallets and explorers to implement for every program. |
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.
App mutability is an important factor in a rational actors decision making when it comes to interacting with an app and sending funds to a contract account. If a contract is mutable, it is possible that funds could be stolen or lost after the initial program is changed or deleted.
I'm not really sure I see the point of this ARC as even an immutable contract could steal your funds if you don't understand all the code. It might just provide a false sense of security.
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.
There is no way to have 100% security guarantees. A bug in algod could result in loss of funds, but that doesn't mean we should discount all other security considerations.
if you don't understand all the code
The goal of this ARC is to make it easier to understand critical parts of the code.
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.
Ok, but what about existing immutable contracts that don't have this code pattern and can't be updated to use it. They miss out on this 'trusted' badge.
At first glance it also doesn't look like this pattern will work with the existing beaker template variables for TMPL_UPDATABLE and TMPL_DELETABLE. Obviously there are alternative solutions to this - perhaps this ARC should describe them?
I think ultimately this would be better resolved at the AVM level with a new opcode/pragma comment at the top of the program.
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.
Support the idea of dealing with this at avm level. I have seen chat recently about legal issues of mutable contracts, so the requirement seems to be one of some consequence.
This app provides standardized TEAL that a developer can use to allow tools such as explorers or wallets easily determine if an app can be updated or deleted (among other actions)