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

ARC-64: AVM Run Time Errors based on program source map #305

Closed
wants to merge 8 commits into from

Conversation

cusma
Copy link
Contributor

@cusma cusma commented Aug 5, 2024

This ARC formalizes the conventions, already in use, to rise AVM run time errors based on Program Counter Source Map.

Copy link
Contributor

@joe-p joe-p left a comment

Choose a reason for hiding this comment

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

Am I correct in saying that this ARC is only being proposed to formalize what currently exists in Algokit? If so, then I question whether it's worth creating an ARC when we will have a better formal standard with ARC56. If the goal is to have an ARC that can also be used going forward, then I would have more comments to make.

- No opcode budget required to rise errors;
- No program size consumed by error messages.

### CONS
Copy link
Contributor

Choose a reason for hiding this comment

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

Another con here is that using the PC will be unreliable for contracts that have template variables. Integer or dynamic byteslice template variable values will change the PC. This is why ARC56 proposes using disassembled TEAL lines in the event template variables. Regardless of the width of the template variable values, the disassembled TEAL will always be the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

In our side conversation about this you said the steps required to make use of this (from front-end perspective) are:

  1. Get pc=XXX error
  2. Pass bytecode to /dissassemble to get TEAL
  3. Pass TEAL from 2 to /compile?sourcemap=true
  4. Lookup the PC XXX and see what line it corresponds with
  5. Find the object in the source map with disassembledTeal that matches the line given in step 4

Given a front-end will just get a pc=xxx error (that it has to parse out), for it to have to do all that (assuming the node its connecting to even allows the disassemble/compile endpoints!) just to get the error message associated with that PC is extremely onerous. I think it kills the whole thing imo.

Short of dynamic strings, I think the template variables being variable sized is a fixable issue. Instead of having templates do pushint, pushbytes etc at every reference, perhaps have them initialized at start into scratch storage from fixed size values, ie: btoi(0x0000000000000001) for a templated int 1 - and then reference that scratch slot late for the constant reference?

@cusma
Copy link
Contributor Author

cusma commented Aug 6, 2024

Am I correct in saying that this ARC is only being proposed to formalize what currently exists in Algokit? If so, then I question whether it's worth creating an ARC when we will have a better formal standard with ARC56. If the goal is to have an ARC that can also be used going forward, then I would have more comments to make.

@joe-p the scope of this ARC is twofold:

  1. Formalizing what is already a very common practice in the ecosystem, early used by PyTeal and now also adopted by the Puya compiler. There was never a formal spec to formalize this and I think the client-side spec provided by ARC-56 should not be mandatory if you just want to have a convention for AVM run time error messages. Example: a compiler could conform to this conventions (as Puya is doing) but (still) not generate ARC-56 conforming App Specs.
  2. I'm working with @tasosbit and @nullun an alternative AVM run time error convention based purely on the program (not on program Source Map). In this way we can have both approaches well formalized and a compiler/language could chose which one support (if not both), and this is not related to the App Spec, so I think we should abstract this conventions from ARC-56 (which could inherit this one, as it proposes a standard way to provide the program error Source Map).

@joe-p
Copy link
Contributor

joe-p commented Aug 6, 2024

Formalizing what is already a very common practice in the ecosystem, early used by PyTeal and now also adopted by the Puya compiler. There was never a formal spec to formalize this and I think the client-side spec provided by ARC-56 should not be mandatory if you just want to have a convention for AVM run time error messages.

I think this ARC is missing some context then. It's not really clear what the source map standard actually is. Is the standard being proposed the [pc=X] lines in TEAL? If so, then that requires an additional artifact to be produced by compilers that does not already exist and for all clients to have that TEAL available. Or is the standard being proposed assuming the clients have the original TEAL source available and source mapping done by the client via the algod endpoint? If so I think that should be noted in the ARC.

Example: a compiler could conform to this conventions (as Puya is doing) but (still) not generate ARC-56 conforming App Specs.

I suppose it's not really clear to me why someone would want this when in any scenario you'd still need to provide special artifacts from the compiler to clients.

@cusma
Copy link
Contributor Author

cusma commented Aug 6, 2024

Or is the standard being proposed assuming the clients have the original TEAL source available and source mapping done by the client via the algod endpoint? If so I think that should be noted in the ARC.

This! The proposed spec is not really for the program "client", it is rather for the program itself. It is a formalization of what the TEAL (either handwritten or generated by a compiler) should look like if it wants to conform to this AMV run time error messages conventions. It really says nothing about the auxiliary App Spec (which is what ARC-56 is solving for).

@cusma cusma changed the title ARC-64 - AVM Run Time Errors based on Source Map ARC-64: AVM Run Time Errors based on program source map Oct 9, 2024
@cusma
Copy link
Contributor Author

cusma commented Nov 22, 2024

Withdrawn in favour of the #258 approach to AVM errors based on source map.

@cusma cusma closed this Nov 22, 2024
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.

3 participants