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

Treat ALGO as ASA with id 0 #4261

Closed
1m1-github opened this issue Jul 13, 2022 · 23 comments
Closed

Treat ALGO as ASA with id 0 #4261

1m1-github opened this issue Jul 13, 2022 · 23 comments
Labels
new-feature-request Feature request that needs triage Team Scytale

Comments

@1m1-github
Copy link

Problem

Lots of code with the same logic needs to be duplicated just because ALGO opcodes are separate from ASA opcodes.

E.g. checking whether sent # of coins is at least T, no matter which coin (ALGO or another ASA), we have to branch and check for 2 cases.

This is lots of code duplication, with a significant long term impact on chain storage size.

Also, longer codes means more bugs.

The following changes would be examples for opcodes (unification)
Sender -> AssetSender
Receiver -> AssetReceiver
CloseRemainderTo -> AssetCloseTo
Amount -> AssetAmount

It would simplify the documentation by reducing opcodes. Simpler is better, especially for low level assembly.

Solution

Treat ALGO as yet another ASA, the one with id 0.

The solution needs to unify ALGO as an ASA in the go sdk, assert that fee ASA id == 0 for all txns.

suggested here: https://discord.com/channels/491256308461207573/806903824265904149/989617511676932127

Dependencies

Via pragma, both old and new code can be guaranteed to function correctly.

Off-chain analysis tools would need to potentially update their code. Temporary wrong displayed data on such web2 apps will be fixed but more important, the actual data on web3 will always be safe.

Ideally, such a change comes via governance.

Urgency

The earlier such a change is made, the better. Given the size of the change, it should be made ASAP.
Even if now feels like "too late", it will be much better than in 5 years.

@1m1-github 1m1-github added the new-feature-request Feature request that needs triage label Jul 13, 2022
@algorand algorand deleted a comment Jul 19, 2022
@2i2i
Copy link

2i2i commented Nov 5, 2022

the one case where i see having ALGO treated specially is if Algorand could be used to create separate chains (e.g. private chains) ~ separate chains would have separate ASA universes ~ forces all Algorand chains to use a common ALGO coin (gov and fee coin) allows ALGO to benefit from the dev of separate chains

edit: btw, this is OP ~ i used two diff accs without noticing

@algoanne
Copy link
Contributor

algoanne commented Jan 3, 2023

Hi,

this is an oft-requested and challenging issue, and as such has been on my mind for a while now. I'd like to discuss, so I have laid out some thinking and possibilities below.

How ALGO is used differently from other assets

  • Transaction fees are paid in ALGO.
  • Minimum Balance Reqs are in ALGO.
  • Consensus participation is based on ALGO.
  • Governance participation is based on ALGO.

Some considerations when evaluating options

  • How complex is the implementation? (how risky is it?)
  • Will refactoring be required by apps? How much? On what timeline?
    • Will exchanges need to refactor?
    • How will this affect TVL and Market Cap measurements (by e.g. Defillama) and if affected, can these be fixed easily?
  • Will this affect protocol performance?
  • Will this affect end user experience?
  • Could this affect tokenomics?

Option A: Do nothing, apps use workarounds

Wrap the ALGO as wALGO, which is then an asset that can be used for everything like a normal asset. “Wrap” means there is a contract that swaps ALGO for wALGO 1:1 in either direction unconditionally.

Impact

A dapp could now just support wALGO and not have to worry about ALGO, except to cover fees and MBR. Some logic in the app would no longer need to be duplicated: for example, a Defi AMM contract for wALGO/USDC would look the same as USDC/CHIPS.

On the other hand, dapps still have to worry about ALGOs and pay txns for paying fees and MBR.

Defi apps (e.g. AMMs) still have to support ALGO because people will still need ALGO to cover their MBRs and fees.

The end user UX might be not great: when users use an app that requires wrapping their ALGO, the first thing they need to do is an extra app call to wrap their ALGO before they get to use the app.

And lastly, If everyone does not center around the same wALGO then it has the potential to be confusing.

Option B: Just do it

10B ASA 0 are minted.

ASA 0 are distributed to each address holding ALGO in a 1-to-1 transfer, with no fees incurred for these transfers.

Afterwards:

  • Protocol no longer accepts pay transactions.
  • Fees are now covered by ASA 0
  • MBR is now covered by ASA 0
  • Consensus participation is now based on ASA 0
  • Governance participation is now based on ASA 0

Impact

Any app that previously existed on Algorand now needs to refactor. Every single app is affected, because every app has an MBR and uses txn fees.

Without enough advance warning, this has the potential to devastate the ecosystem (from my perspective). Unfortunately, "enough advance warning" might be a very, very long time. How do we convince all the big players to put in the work to refactor on our timeline?

"best of both worlds?" options

Option C: Do both

Mint and distribute ASA 0 to every address that has ALGO. Old apps still work as is and new apps don’t need to worry about the old system.

Whenever ASA 0 is transferred, a corresponding pay transaction happens.
Whenever ALGO is paid, a corresponding axfer happens.

Option D: Pretend you did it

When someone submits an axfer 0, the AVM reinterprets it as a pay. There is no actual ASA 0.

Pays continue to work as is.

Option E: Do it, but pretend you didn’t

ASA 0 is now used for fees/MBR/consensus.

When someone submits an ALGO pay, the AVM reinterprets it as an ASA 0 axfer.

Impact

In options C/D/E, we do achieve our goal of allowing new apps to ignore ALGO entirely and just use ASA 0.

The consequence is that we have added a significant amount of logic and computational overhead to every single transaction. Lots of room for error. None of these present a long-term way off to a one-ALGO solution.

Option F: Do both, but with a transition path

Everyone can now trade in their ALGO for ASA 0 at any time at no cost from a central contract.
They can never go back to ALGO using this central contract (but can still trade back through other AMMs if they want). ASA 0 and ALGO now count for fees, MBR, consensus (just add the two values together).

Impact

Existing dapps still work, but over time will either add their own dual-support or migrate entirely to ASA 0 and require their users to use ASA 0.

Exchanges continue to work - from their perspective, the supply of ALGO is decreasing over time. They eventually support ASA 0 as just another asset. Exchanges who already support assets are all set.

TVL and MC measurements may get screwed up, but theoretically that can be fixed.

This has the potential for weird tokenomics, as the supply of ASA 0 grows and the supply of ALGO shrinks in proportion or out of proportion with the set of apps that are usable in each. This is not necessarily bad, but it's something I personally don't know how to predict and feels like a risk. Perhaps setting a real sunset date for ALGO a couple years down the line solves this uncertainty.

Discussion

Does anybody have any other ideas not listed above?

Do you disagree with any of the statements made above?

Now that we understand some of the risks involved, do any of these paths seem worth it?

@joe-p
Copy link
Contributor

joe-p commented Jan 3, 2023

@algoanne's option D makes the most sense to me if we were to go with any of them.

However, thinking about what really are the points of friction here it's mostly about the txn fields and language ergonomics surrounding them.

Some other options:

  1. Just have algod/AVM know that Receiver for a axfer is really AssetReceiver. The txn type would still be an explicit axfer or pay, but axfer accepts fields named in-line with pay.

  2. SDKs and TEAL could use Receiver for axfers but the underlying request/txn/bytecode uses AssetReceiver. So mostly the same as 1, but "translation" is done by the compiler/SDKs instead of AVM.

Edit: Also having gtxnss and itxn_fields opcodes for getting the txn field enum from stack rather than immediate could also simplify code. But the above two options seem better

@jeapostrophe
Copy link

Whenever I've thought about this, I've wanted option D. To me, this is only about making programming convenient. It is annoying to have two code paths for ALGO and ASA in DeFi applications. I just want there to be an ASA that is "actually the ALGO" from a programming perspective. I think this is just a bunch of special cases in the code when id=0

@2i2i
Copy link

2i2i commented Jan 3, 2023

i also think option D would be enough and much easier vs the other options

edit: btw, this is OP ~ i used two diff accs without noticing

@pbennett
Copy link

pbennett commented Jan 3, 2023

I upvoted, but to make it clear, option D seems the cleanest way to me as well - and is how I implement things as well. For an upcoming change I treat an asset transfer of 0 as an ALGO transfer and have different itxns based on it. If I could just treat it as an ASA for everything that'd be perfect.
Obviously the opt-in and balance checks need to work correctly with ASA 0 as well.

@SilentRhetoric
Copy link

I support Option D and have already written smart contract code to handle this dilemma this way. This is primarily a programming problem and so should have a programming solution.

Doing some kind of mass conversion of Algo to ASA 0, requiring all users of the network to change their behavior, is a recipe for chaos and confusion that will annoy (I'm being nice) the user base. If you've ever been in a country that changed its currency, that experience is a fitting analogy.

@algoanne
Copy link
Contributor

algoanne commented Jan 5, 2023

@pbennett and @SilentRhetoric can you explain a little more what you mean when you say you're already doing this?

I like @joe-p 's suggestion as a light improvement that isn't too obfuscating.
Could perhaps also do it for Sender and Amount as @1m1-github suggests in the original issue comment. I don't know about CloseRemainderTo / AssetCloseTo since CloseRemainderTo for Algo has the other consequence of closing the sender account.

@joe-p
Copy link
Contributor

joe-p commented Jan 5, 2023

Also, if we do add some "aliases" for the txn fields, I'd propose that we alias AssetSender as ClawbackTarget or something along those lines to make it clearer. Not related to this issue directly but if we're making QoL changes to the txn fields might be useful to clarify this as well

@SilentRhetoric
Copy link

SilentRhetoric commented Jan 5, 2023

@pbennett and @SilentRhetoric can you explain a little more what you mean when you say you're already doing this?

Subroutine(TealType.none)
def send_algo_or_asa(asset_id: Expr, amount: Expr, account: Expr):
    """Builds an InnerTxn dynamically as a payment or asset transfer
    depending on the asset_id

    Args:
        asset_id: A uint64 which can be 0 to represent ALGO or a valid ASA ID
        amount: A uint64 quantity amount for the payment or asset transfer
        account: The receiving account as a PyTeal expression
    """
    return Seq(
        If(asset_id == Int(0))
        .Then(
            Seq(
                InnerTxnBuilder.Begin(),
                InnerTxnBuilder.SetFields(
                    {
                        TxnField.type_enum: TxnType.Payment,
                        TxnField.amount: amount,
                        TxnField.receiver: account,
                        TxnField.fee: Int(0),
                    }
                ),
                InnerTxnBuilder.Submit(),
            )
        )
        .Else(
            Seq(
                InnerTxnBuilder.Begin(),
                InnerTxnBuilder.SetFields(
                    {
                        TxnField.type_enum: TxnType.AssetTransfer,
                        TxnField.xfer_asset: asset_id,
                        TxnField.asset_amount: amount,
                        TxnField.asset_receiver: account,
                        TxnField.fee: Int(0),
                    }
                ),
                InnerTxnBuilder.Submit(),
            )
        ),
        Log(Bytes("InnerTxn sent")),
    )

@pbennett
Copy link

pbennett commented Jan 5, 2023

@pbennett and @SilentRhetoric can you explain a little more what you mean when you say you're already doing this?

I have contract calls where the asset to 'send' is a Itob 64-bit int. My convention is an ID of 0 is passed if ALGO is meant to be sent instead of a specific ASA. I then have conditional logic in the contract to issue a pay or axfer based on the id being 0 or not (and balance checks, etc. as appropriate).
Looks like @SilentRhetoric replied with code example of the same thing.

So it just being 'built-in' - where we issue asset operations but asset id 0 is just automatically a pay - would be nice - and would reduce contract size as well.

@fergalwalsh
Copy link

I definitely want a solution for this so thank you for raising the issue and putting some thought into it.

I think I like option D but need to clarify that it does what I think;

When someone submits an axfer 0, the AVM reinterprets it as a pay. There is no actual ASA 0.

My understanding:
In our protocols that handle Algo & ASAs for similar purposes we would specify that Algo transactions MUST be made with axfer with index=0. In the contract code we would always use Txn.AssetReceiver & Txn.AssetAmount and similar opcodes.

Where Algo and only Algo is expected we specify a pay transaction is required and continue as normal with Txn.Receiver & Txn.Amount.

Am I understanding correctly? If so then yes, that would be beautiful and remove significant amounts of repetitive code from many contracts.

Ideally 0 support would also be added to asset_holding_get & asset_params_get so we don't have to special case those ops either.

I think all of this would have to be only allowed for contracts using a future version. Existing contracts should fail if any txn in the group uses axfer with index=0. I think this is necessary because contracts could be doing internal accounting of balances in state and would only expect one way of modifying the Algo balance (with actual pay).

Indexers, wallets, etc would have to be updated to handle axfer with 0 but it doesn't seem like it would be too much trouble.

@pbennett
Copy link

pbennett commented Jan 6, 2023

Indexers, wallets, etc would have to be updated to handle axfer with 0 but it doesn't seem like it would be too much trouble.

I would think this would all happen transparently as part of the itxn handling - and it would still just be a pay of algo, or an axfer of an ASA.

@aurel-fr
Copy link

I like A.
Option D is satisfactory but I'd be very careful with it. For example a contract that loads the assetID from a scratch slot for an inner tx could expect to fail if for reason X, Y or Z that value wasn't set in memory. Now after the change, when that contract reads from empty memory and loads 0, instead of the inner tx failing it would turn into a pay transaction! Just thinking about edge cases here. If algo is asa 0 how do we differentiate an empty memory slot from a slot whose value was set to 0?

@fergalwalsh
Copy link

@algoanne could you please expand on the details of option D? There seems to be different interpretations of it here in the comments.

@emg110
Copy link
Contributor

emg110 commented Jan 11, 2023

I gladly go with option D!
1- Unification of opcodes for itxns regarding ALGO and ASA is mostly appreciated (The only differentiation to be TYPE ENUM only). this significantly reduces the TEAL code lines needed!
2- treating ALGO as ASSET ID 0, during interpretation is the safest way in my opinion, given the solid and predictable flawless AVM memory management!
And a big thank you for the amazing response planning scenario structure @algoanne

@FrankSzendzielarz
Copy link

FrankSzendzielarz commented Jan 11, 2023

My feeling is there are several conceptual issues in the Transaction model, including the above overlap between Payment and AssetTransfer, and the above could be addressed in a broader re-shape of how Transactions are represented:

  • Is there any conceptual difference between a Payment and an Asset Transfer in the real world? Are there significant legal or financial differences that warrant two entities in the Transaction scheme?

  • The documentation states that RekeyTo is a distinct transaction type yet modeled specifically as a type of Payment. Why?

  • An "Axfer" is an abstract transaction type, distinct from an "acfg" type in that asset configuration transactions (change, create, update, destroy) work with Asset entities, while asset transfer (axfer) deals with the Asset - Account relationship (transfer, clawback, optin). Yet we have Asset Freeze as a separate transaction type, despite that it is also about Asset - Account relationships.

  • We have FreezeAsset(faid) and XferAsset(xaid) as separate fields. Similar with fadd and arcv. Why?

  • Why are transaction "types" only abstract in some cases, and concrete in others, with the only way to determine an actual transaction type by inspecting the set/unset fields?

In general, I feel that this thread boils down to a philosophical "Is Algo an Asset?" question and that this question belongs to a broader task of redefining the conceptual model and cleaning up the resulting definitions/codes/docs.

@algoanne
Copy link
Contributor

Hi everyone,

Thank you for all your inputs!

Backing up for a second, I think there are two core challenges we're trying to address here:

  1. having these two different transaction types means added complexity in the code that dapp developers are writing. Makes it harder to reason about and develop. More opportunity for bugs.
  2. it makes TEAL programs longer.

(1) is probably the main one you all are thinking about, but I want to address (2) first: at worst this would make programs twice as long - that would be bad. In reality, it seems to me like it should only make programs some O(1) longer, since most of the program logic isn't switching on transaction type. But you tell me if I have that wrong.

As for (1). One way we could potentially accomplish option D is by creating a virtual transaction type that becomes specified by its fields. This would be some new concept of a (virtual) value transfer transaction. In TEAL, this feels somewhat related to the macros we’re introducing in #4737, similar concepts and so maybe achievable in a similar way.

@jasonpaulos suggests we could do something like the following:

// 1 for pay or axfer, 0 otherwise
txn IsTransfer

// For axfer, resolves to txn XferAsset
// For all others, resolves to 0
txn TransferAsset

// For pay, resolves to txn Amount
// For axfer, resolves to txn AssetAmount
// For all others, resolves to 0
txn TransferAmount

// For pay, resolves to txn Receiver
// For axfer, resolves to txn AssetReceiver
// For all others, resolves to 0 address
txn TransferReceiver

These virtual fields could be used to inspect transactions in the current group and to create inner transactions.

That said, there are a myriad of interactions at play here, potential pitfalls and edges… several such subtleties are mentioned in the comments in this thread, but I suspect they only scratch the surface, given the conversations I’ve had with the Inc engineers. The risk here also means this would probably take a lot of work to get correct.

We could instead solve (1) at the level of pyteal or beaker (or other analogues) with utility functions so that developers don’t have to think about the two different transaction types. It seems to give us the same outcome for dapp developers.

The advantages of solving it at the beaker level are that it’s less risky, and thus also less effort, and thus means we (the Inc devs, I mean) can spend our time working on some of the other AVM improvements we want to make instead (such as allowing larger program sizes, smart sigs as inner txns, contract-to-contract view functions, etc.).

@1m1-github
Copy link
Author

i also think code import can mostly give the same result as D ~ maybe that is what "contract-to-contract view functions" mean?

@FrankSzendzielarz
Copy link

@algoanne just to add that while the OP is talking about using Asset Transfer to move Algo, your quote above is not limited to 2 tx types but 4 (payment, optin, transfer and clawback)

@emg110
Copy link
Contributor

emg110 commented Jan 12, 2023

+1 to #4261 (comment)
Works for me!

@FrankSzendzielarz
Copy link

@algoanne Regarding the Beaker/SDK/AlgoStudio level solution. That's what I am currently doing to some extent .

  • The .NET SDK Transaction model incliudes concrete types. Eg: There are AssetOptinTransaction and AssetClawbackTransaction transaction types. Instantiating and sending these is not a problem.
  • Deserialisation was complicated by the fact that the transaction type field cannot be used as a discriminator to identify transaction type. The JSON serialiser had to be enhanced to allow more complex discriminators. When asking Algod API for Transactions or pending Transactions, the returned JSON is passed to this JSON serialiser for deserialisation into the concrete type.
  • This issue will surface for users of Conduit, Indexer or other reporting solutions - any analysis of transactions requires some more complex logic to identify whether a Payment was really a payment (and not, say a clawback, or optin). I would imagine that this will be solved by a standardised Conduit plugin that parses out the underlying fields and wraps transactions into a new schema that more accurately describes the Transaction object model.

My feeling is that applying wrappers and translators in the assembler, in Conduit, in the SDKs is creating work to achieve a short term solution to a hard problem. It feels hacky. My suggestion would be to bite the bullet and restructure all the transactions, fields and field names into a better model.

@algoanne
Copy link
Contributor

Alright, @barnjamin is into doing this at the Beaker level: algorandfoundation/beaker#170

@1m1-github I don't fully understand the connection/implication but would be happy to talk about code import in discord. It's something we've discussed doing, so I'd love to hear what exactly you're imagining and why it would be useful for you.

Lastly, @FrankSzendzielarz I was kind of expecting someone to make the argument "put it at the lowest level so that everyone benefits". But of course, if everything's at the lowest level, then nothing's at the lowest level and we run the risk of building a big bloated mess. In this situation, based on everything I've heard, I think we should make the call not to implement it at the AVM level.

If new perspectives come to light we should as always re-discuss. Thanks again to everyone for your inputs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature-request Feature request that needs triage Team Scytale
Projects
None yet
Development

No branches or pull requests

13 participants