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

[Ready for decision]: ADR for arc4 by default #5

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

tristanmenzel
Copy link
Collaborator

No description provided.


At its bare minimum, a smart contract on the Algorand Block Chain consists of an approval program and a clear state program. Smart signatures (also known as logic signatures) consist of only an approval program. For signatures the approval program is run to determine in a transaction signed by the program should be considered valid. For contracts the clear state program is invoked for Application Call (`appl`) transactions which have an on completion action (`apan`) set to clear state (`3`). The transaction is committed to the chain regardless of the outcome of a clear state program. For all other on completion actions the approval program is invoked. It is up to the approval program to inspect the current transaction's properties to determine the outcome of this transaction.

[ARC4](https://github.com/algorandfoundation/ARCs/blob/main/ARCs/arc-0004.md) introduces conventions for encoding data passed to a smart contract, and for routing execution to specific subroutines within the approval program based on key properties of the Application Call transaction (eg. The on completion action and application args). It is the current standard for developing smart contracts on Algorand, but may not always be. There are several other ARCs such as [ARC32](https://github.com/algorandfoundation/ARCs/blob/main/ARCs/arc-0032.md), [ARC56](https://github.com/algorandfoundation/ARCs/blob/e540d921502f19c720b64d8df1f09563158ca348/ARCs/arc-0056.md), and [ARC28](https://github.com/algorandfoundation/ARCs/blob/main/ARCs/arc-0028.md) which build on, expand, or complement ARC4.
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth calling out that we have seen feedback from some developers who for various reasons chose not to use ARC4 in certain circumstances.

Copy link
Contributor

Choose a reason for hiding this comment

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

But most of those developers choose not to use ARC4 because it sucked to use with Pyteal and made their lives harder. A good example is NFDomains. NFDs is currently not ABI-compliant for the aforementioned reason, but their v3 is being rewritten in TEALScript with ABI compatible methods. There is basically no downside to using the ARC4 method routing. You can make all of your arguments byte[] and treat them however, you'd like. The important thing is that by following ARC4 you can leverage ARC4 tooling and composability.

## Requirements

- It must be possible to define a contract with ARC4 routed methods
- It must be possible to define a contract which doesn't use ARC4 routing
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should include a why for these or if it's obvious? For this one the why would centre around the fact that Puya as a compiler architecture is agnostic of ARC-4 (but also has first class ARC-4 support of course) and we are aiming to give developers a flexible experience that let's them use the Algorand Blockchain capabilities in whatever way they want (while providing a first-class ARC-4 experience too that helps them fall into the pit of success).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the main reason why developers would want non-ABI support is so they can migrate existing contracts. This is why TEALScript supports non-ABI. It initially was only ABI, but NFDomains wanted to use TEALScript without having to break all of their infra. For new developers, I don't see a compelling reason why they'd choose not to use ARC4.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are building languages for devs to use, they will want to do all kinds of weird and wonderful things we can never imagine so providing flexibility it’s important. That aside, I agree we want to make ARC-4/32/56 the default, and easy, to guide developers to fall into the pit of success so we should make it really easy I agree.

@Loedn
Copy link
Contributor

Loedn commented Jun 7, 2024

ARCs come and go, the AVM is here forever. I'm in between 2-3 but for compatibility and ease of documentation with algopy I would go with 2

@robdmoore
Copy link
Contributor

avm4lyfe :P

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.

Do we want to also mention "action" (create/OC) selection here or is that a separate ARC?


At its bare minimum, a smart contract on the Algorand Block Chain consists of an approval program and a clear state program. Smart signatures (also known as logic signatures) consist of only an approval program. For signatures the approval program is run to determine in a transaction signed by the program should be considered valid. For contracts the clear state program is invoked for Application Call (`appl`) transactions which have an on completion action (`apan`) set to clear state (`3`). The transaction is committed to the chain regardless of the outcome of a clear state program. For all other on completion actions the approval program is invoked. It is up to the approval program to inspect the current transaction's properties to determine the outcome of this transaction.

[ARC4](https://github.com/algorandfoundation/ARCs/blob/main/ARCs/arc-0004.md) introduces conventions for encoding data passed to a smart contract, and for routing execution to specific subroutines within the approval program based on key properties of the Application Call transaction (eg. The on completion action and application args). It is the current standard for developing smart contracts on Algorand, but may not always be. There are several other ARCs such as [ARC32](https://github.com/algorandfoundation/ARCs/blob/main/ARCs/arc-0032.md), [ARC56](https://github.com/algorandfoundation/ARCs/blob/e540d921502f19c720b64d8df1f09563158ca348/ARCs/arc-0056.md), and [ARC28](https://github.com/algorandfoundation/ARCs/blob/main/ARCs/arc-0028.md) which build on, expand, or complement ARC4.
Copy link
Contributor

Choose a reason for hiding this comment

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

But most of those developers choose not to use ARC4 because it sucked to use with Pyteal and made their lives harder. A good example is NFDomains. NFDs is currently not ABI-compliant for the aforementioned reason, but their v3 is being rewritten in TEALScript with ABI compatible methods. There is basically no downside to using the ARC4 method routing. You can make all of your arguments byte[] and treat them however, you'd like. The important thing is that by following ARC4 you can leverage ARC4 tooling and composability.

## Requirements

- It must be possible to define a contract with ARC4 routed methods
- It must be possible to define a contract which doesn't use ARC4 routing
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the main reason why developers would want non-ABI support is so they can migrate existing contracts. This is why TEALScript supports non-ABI. It initially was only ABI, but NFDomains wanted to use TEALScript without having to break all of their infra. For new developers, I don't see a compelling reason why they'd choose not to use ARC4.


Cons:
- The current implementation in TealScript outputs a lot of redundant teal in scenarios where an advanced user wants to do their own routing
- Doesn't give us a low friction path forward if ARC4 were to be replaced, or significantly updated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the chance of ARC4 getting replaced or significantly updated is close to zero. There's a graveyard of ARCs attempting to do so as evidence lol. Also, ARC4 is such a low level detail that has such strong effects up the entire chain of infrastructure that a change to ARC4 is essentially a breaking change to everything. Thus if we want to support a new ABI, having a backwards incompatible way of doing that isn't unreasonable in my opinion.

To be clear, there's a difference between changing the existing ABI and building on top of it. Maybe we have a new type timestamp that is used as a timestamp, but in the ARC4 signature it is still uint64 and encoded as such. timestamp would be reflected at higher levels like the language and ARC56. These sort of changes are possible without having to break anything.

architecture-decisions/2024-06-06_arc4-by-default.md Outdated Show resolved Hide resolved
architecture-decisions/2024-06-06_arc4-by-default.md Outdated Show resolved Hide resolved
@joe-p
Copy link
Contributor

joe-p commented Jun 7, 2024

ARCs come and go, the AVM is here forever. I'm in between 2-3 but for compatibility and ease of documentation with algopy I would go with 2

I think the ARC4 is not just any regular ARC. It's the ABI. Any changes, even incredibly small ones, would have massive ripple effects on the ecosystem and have negative implications on composability and compatability. There is a graveyard of attempted amendments to the ABI for a reason. Also looking at other ecosystems, blockchain or otherwise, changing ABIs are rare and almost never a user-facing issue.

Within the language we can always support things on top of the ABI, but rarely should we try to change it (speaking as someone that has previously tried to change it). To quote a review comment I made

To be clear, there's a difference between changing the existing ABI and building on top of it. Maybe we have a new type timestamp that is used as a timestamp, but in the ARC4 signature it is still uint64 and encoded as such. timestamp would be reflected at higher levels like the language and ARC56. These sort of changes are possible without having to break anything.

If there were changes to the ABI, this should be a compiler option, not language syntax. How many people have written code that runs on Intel Macs and Apple Silicon Macs? A lot. How many of those developer even know what an ABI is? Probably a small percentage of them. Is the ABI used important? Of course, but I don't think it should impact the language.

@joe-p
Copy link
Contributor

joe-p commented Jun 7, 2024

Also I want to note the fact that decorator-based approach was the way TEALScript initially handled ABI routing. I got feedback from TypeScript developers that this felt foreign to them and I myself was initially against the current ABI-by-default approach TEALScript uses now which avoids decorators for most contracts, but it's what developers expressed as being the preferred way of doing things and I now tend to agree.


```

An ARC4 compliant contract would instead extend the `algopy.arc4.ARC4Contract` base class. Each method must be decorated with either `@abimethod` or `@subroutine` to indicate a public or private method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
An ARC4 compliant contract would instead extend the `algopy.arc4.ARC4Contract` base class. Each method must be decorated with either `@abimethod` or `@subroutine` to indicate a public or private method.
An ARC4 compliant contract would instead extend the `ARC4Contract` base class. Each method must be decorated with either `@abimethod` or `@subroutine` to indicate a public or private method.

The specific characteristics of python that lead to mandatory decorators on all methods do not apply to the TypeScript solution. Whilst still keeping the separation of ARC4 from option 2, we could use TypeScript's `public` and `private` keywords to flag an ABI method versus a private subroutine. The decorator would still be used in cases where a non-default on completion action was required but otherwise optional. We can require explicit access keywords (`public`/`private`/`protected`) on all methods to work around the potential security issues of public by default. This behaviour would only apply when extending `arc4.Arc4Contract`. Access modifiers on a contract which extends the base `Contract` would have no implicit effect.

```ts
export default class DemoContract extends arc4.Arc4Contract {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would propose that we have Contract be ARC4 routing and then have a NonABIContract. Otherwise I like this option

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe AVMContract?

Copy link
Contributor

Choose a reason for hiding this comment

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

AVMContract for ABI or non ABI?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be my suggestion for non-ABI if Contract is the ABI default, it speaks better to what the contract is (direct AVM method implementation)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I don't hate it but I like the explictness of NonABIContract if I see an example with AVMContract and Contract I might erroneously assume Contract is not for the AVM. Or perhaps I'm a developer that uses Contract and see someone use AVMContract and it's not immediately obvious what the difference is.

Copy link
Contributor

Choose a reason for hiding this comment

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

That’s fair. Per the other conversation thread if we decide it makes sense to be ABI by default then NonAbi is clear marker you are deviating from the default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Taxonomically, I think having Contract be a sub-class of anything named FooContract (whatever foo may be) doesn't make a lot of sense, and naming a class for what it's not (ie. NonABI) also smells a bit.

I think we should stick to Arc4Contract for the arc4 contract but maybe we can come up with something descriptive for the 'Base no-frills' contract which is not just Contract as it might alleviate some migration pain for users coming from TealScript (If the name Contract doesn't exist, they'll be forced to look up the new name rather than accidentally importing the base contract type)

Having said that, I think we need to consider that this would be a deviation from Algorand Python - meaning side by side examples of TS and Python will look unnecessarily (IMO) different and require additional explanation. By keeping the two languages as close as possible (with the exception of semantic differences) it makes it easier to author a smart contract blog post/tutorial/workshop where developers can choose their preferred language rather than having to duplicate our efforts and create python and typescript specific media.

Copy link
Contributor

Choose a reason for hiding this comment

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

Taxonomically, I think having Contract be a sub-class of anything named FooContract (whatever foo may be) doesn't make a lot of sense, and naming a class for what it's not (ie. NonABI) also smells a bit.

In general I agree but we have to remember that non-ABI is a very very small percentage of users. If you are going out of the way to make up your own routing I think you'll be able to handle non-orthodox class names.

Having said that, I think we need to consider that this would be a deviation from Algorand Python - meaning side by side examples of TS and Python will look unnecessarily (IMO) different and require additional explanation.

Education is the main I reason I prefer avoiding having ARC4 in the class name. It's something that would require explaining right off the bat with the first line of code. Instead, we should try to limit the amount of new ideas we are exposing to new developers and abstract away the low level details as much as possible when writing a "Hello, world" contract.

By keeping the two languages as close as possible (with the exception of semantic differences) it makes it easier to author a smart contract blog post/tutorial/workshop where developers can choose their preferred language rather than having to duplicate our efforts and create python and typescript specific media.

LLMs have converted PyTEAL to TEALScript effortlessly, so I have full confidence that the effort for conversion between Algorand Python and TypeScript, regardless of the difference, will be effortless. Video-based content inherently requires duplicate effort anyways so I don't think it's much of a concern there.

Perhaps this is something @SilentRhetoric can weigh in on.

Also another option is we just have one class Contract and have a way to implement non-ABI methods like how it currently works in TEALScript. More details are in the conversation with @robdmoore in the main PR thread.

@joe-p
Copy link
Contributor

joe-p commented Jun 7, 2024

Also one thing I just thought of: The chosen option should allow for a mixture of ABI methods and a non-ABI router as this is really the most common use case when people choose not to use the ABI for everything.

@robdmoore
Copy link
Contributor

If you had a non Abi router how would abi methods work?

@joe-p
Copy link
Contributor

joe-p commented Jun 8, 2024

TEALScript does this via a nonABIRouterFallback decorator.

class NonABIExample extends Contract {
  private add(x: uint64, y: uint64): uint64 {
    return x + y;
  }

  abiAdd(x: uint64, y: uint64): uint64 {
    return this.add(x, y);
  }

  @nonABIRouterFallback.call('NoOp')
  nonAbiAdd(): void {
    const x = btoi(this.txn.applicationArgs![0]);
    const y = btoi(this.txn.applicationArgs![1]);
    log(itob(this.add(x, y)));
  }
}

In this example, abiAdd is a ABI method, add is a subroutine, and nonAbiAdd is the method that's executed when no method selector is matched for NoOp calls.

@robdmoore
Copy link
Contributor

I think it’s a fair point that avoiding boilerplate is good as long as it’s intuitive for the developer, which public/private methods are for typescript developers. One of the realisations we had for Algorand Python was that we realised that a contract instance (app) on chain effectively became a contract instance in an object oriented sense so we doubled down on that analogy in the Algorand python docs. If we take the same analogy here then it very much makes sense to use public / private in that way. And in fact, since external contracts can call an abi method it works well.

We also should support protected methods as a way of abstracting reusable subroutines for base classes in the code reuse model.

@robdmoore
Copy link
Contributor

Oh, so you can co-mingle abi and non abi, cool, super interesting feature. Presumably you can only have 1 fallback for a given on-complete? And then maybe a separate bare method too?

@joe-p
Copy link
Contributor

joe-p commented Jun 8, 2024

I think it’s a fair point that avoiding boilerplate is good as long as it’s intuitive for the developer, which public/private methods are for typescript developers. One of the realisations we had for Algorand Python was that we realised that a contract instance (app) on chain effectively became a contract instance in an object oriented sense so we doubled down on that analogy in the Algorand python docs. If we take the same analogy here then it very much makes sense to use public / private in that way. And in fact, since external contracts can call an abi method it works well.

We also should support protected methods as a way of abstracting reusable subroutines for base classes in the code reuse model.

Yeah this is exactly how I wanted developer to think of TEALScript contracts. public, private, protected is nice because every developer understands it.

Oh, so you can co-mingle abi and non abi, cool, super interesting feature. Presumably you can only have 1 fallback for a given on-complete? And then maybe a separate bare method too?

Yeah exactly. For TEALScript I route differently for create+OC and call+OC. You could also define a bare method.

For example, the routing for the above looks like

*call_NoOp:
	method "abiAdd(uint64,uint64)uint64"
	txna ApplicationArgs 0
	match *abi_route_abiAdd

	// !!!! WARNING: non-ABI routing
	callsub nonAbiAdd

Without the fallback it would just err without a match.

@robdmoore
Copy link
Contributor

Makes sense

@joe-p
Copy link
Contributor

joe-p commented Jun 10, 2024

After thinking about it some more do we really benefit from having multiple classes? I think the only tangible con to have a single ABI-by-default class is

  • The current implementation in TealScript outputs a lot of redundant teal in scenarios where an advanced user wants to do their own routing

Which is true in some scenarios, but the amount of overhead is extremely small. This can also be reduced depending on how PuyaTS routes OnCompletes. The weight of complexity and additional things we'd need to document/maintain seems to be much greater than the very very small amount of users that would want direct ApprovalProgram control (and to be honest ,I can't think of a use-case where you would want this).

For the other two cons

  • Doesn't give us a low friction path forward if ARC4 were to be replaced, or significantly updated

As mentioned elsewhere, the chances of this happening seem very low. If we need to target a different ABI, that should be a compiler option.

  • In TypeScript, class members are public by default which could lead to accidentally exposing a method which shouldn't be.

This could easily be solved by forcing public keyword and doesn't seem to really apply to the decision of ARC4 routing by default or not.

@robdmoore
Copy link
Contributor

You could have a design where there is an overridable approvalProgram and if you override it then you lose ARC4 semantics and take full control of routing?

@robdmoore
Copy link
Contributor

And if ARC4 ever got replaced in the future you could have a different base class so I think that's ok (and agreed it's low risk).

@joe-p
Copy link
Contributor

joe-p commented Jun 11, 2024

You could have a design where there is an overridable approvalProgram and if you override it then you lose ARC4 semantics and take full control of routing?

But you can already do that using the decorator-based approach in an ABI-by-default class. Simply don't define any ABI methods and the contract will immediately go to the "fallback".

And if ARC4 ever got replaced in the future you could have a different base class so I think that's ok (and agreed it's low risk).

I feel like targeting a specific ABI makes more sense at the compiler level than language level. Ie if I want to target ARC1337 ABI there's a --arc1337-abi flag.

Ultimately I don't feel strongly about it, it just seems a bit strange to have a class that no one would really use.

@tristanmenzel tristanmenzel changed the title docs: ADR for arc4 by default [Ready for decision]: ADR for arc4 by default Jun 12, 2024
@tristanmenzel
Copy link
Collaborator Author

marking this one as ready as I think option 4 encompasses all the feedback received here

@joe-p
Copy link
Contributor

joe-p commented Jun 12, 2024

Two objections, the first might be outside the scope of this ADR

  1. I'm not a huge fan of using constructor. I think this will add more confusion because the behavior of any given method will actually be split across the method itself and the constructor depending on if it's a create or not. I think it's a more simple to look at a method and know exactly everything that will happen when that method is called by looking at its definition.

  2. What are the benefits of this approach for handling non-ABI routing over the decorator-based approach that TEALScript currently uses? This would be a significant breaking change for TEALScript and it's not entirely clear to me what the value add is here.

@tristanmenzel
Copy link
Collaborator Author

Two objections, the first might be outside the scope of this ADR

1. I'm not a huge fan of using `constructor`. I think this will add more confusion because the behavior of any given method will actually be split across the method itself and the constructor depending on if it's a create or not. I think it's a more simple to look at a method and know exactly everything that will happen when that method is called by looking at its definition.

It's idiomatic typescript to know that if you have a class that represents a contract - the logic that exists in the constructor will be run when the type is created, and it will run before any method you invoke. Even if we were to hide this behaviour - it would still need to exist as class field assignments are hoisted to the constructor so we would need to implement the same thing behind the scenes. Having said that, it's obviously optional to use so if it offends your sensibilities, simply don't make use of it in the contracts you write!

2. What are the benefits of this approach for handling non-ABI routing over the decorator-based approach that TEALScript currently uses? This would be a significant breaking change for TEALScript and it's not entirely clear to me what the value add is here.

I don't find the nonABIRouterFallback behaviour to be particularly self explanatory as I had to check the generated teal to see what it does. It's restrictive in that you would need to add a 'fallback' for each on completion action (and another for the create scenario(s)). It doesn't allow you to return a value. Perhaps most importantly, its implementation isn't compatible with puya's router generation.

I think unfortunately there will need to be a few breaking changes when moving from TealScript but at least this one is to a feature that nobody uses 😉

@joe-p
Copy link
Contributor

joe-p commented Jun 12, 2024

Having said that, it's obviously optional to use so if it offends your sensibilities, simply don't make use of it in the contracts you write!

The whole point of this product, and this ADR in particular, is to design a language that helps developers "fall into the pit of success". If we support constructors, a logical question would be "why does this contract have some create logic in the constructor and some create logic in this method?". The answer to that may be, "no reason". If we find ourselves creating a product that has multiple ways of doing things with no clear reason to do it one way or the other than that will lead to confusion. This has been common feedback on various developer tools within the ecosystem.

I don't find the nonABIRouterFallback behaviour to be particularly self explanatory as I had to check the generated teal to see what it does.

Agreed, but I think there is no way to this in a self-explanatory way because ABI routing (or lack thereof) is a rather complex concept.

It's restrictive in that you would need to add a 'fallback' for each on completion action (and another for the create scenario(s)).

One fallback can handle multiple actions

It doesn't allow you to return a value.

What users need this?

Perhaps most importantly, its implementation isn't compatible with puya's router generation.

My understanding is that one of the takeaways we will get from PuyaTS is learning how the backend needs to change to be more flexible so various frontends can integrate it.

I think unfortunately there will need to be a few breaking changes when moving from TealScript

Agreed and I have no problem with it if it provides value to actual users.

at least this one is to a feature that nobody uses 😉

Luckily the current mechanism in TEALScript was designed with constant input from one of the developers using this feature who happens to building some of the most important protocols and infrastructure in the ecosystem using TEALScript. I have no problem with breaking changes provided they can provide value for actual users. In this case we'd be doing a breaking change where the end-user value is not clear to me. I'm open to being convinced otherwise. As I said, I initially didn't want to support this feature at all but I listened to the users and built features so they can do what they needed to.

@robdmoore
Copy link
Contributor

The whole point of this product, and this ADR in particular, is to design a language that helps developers "fall into the pit of success". If we support constructors, a logical question would be "why does this contract have some create logic in the constructor and some create logic in this method?". The answer to that may be, "no reason". If we find ourselves creating a product that has multiple ways of doing things with no clear reason to do it one way or the other than that will lead to confusion. This has been common feedback on various developer tools within the ecosystem.

There's two things here:

  1. Conceptually we have a mental model that a deployed app is equivalent to an Object Oriented instance of the contract class, so a constructor makes a lot of sense in that mental model since it's an idiomatic part of an object oriented class
  2. It's useful in that it allows you to define logic that will always be run on creation regardless of different create method(s) you may have.

I think there is no way to this in a self-explanatory way because ABI routing (or lack thereof) is a rather complex concept.

Personally, I think having a onUnmatchedRoute fallback that you can override, but throws an error by default is pretty easy to understand and is quite intuitive as an escape hatch. It also doesn't suffer from the non-obvious behaviour of defining a router override for the same on-complete multiple times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants