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-0200 draft #223

Merged
merged 38 commits into from
Sep 4, 2023
Merged

ARC-0200 draft #223

merged 38 commits into from
Sep 4, 2023

Conversation

temptemp3
Copy link
Contributor

No description provided.

ARCs/arc-0200.md Outdated Show resolved Hide resolved
@joe-p
Copy link
Contributor

joe-p commented Jul 5, 2023

If one of the goals is to mirror ERC20 to presumably allow for seamless bridging between the two chains, it would make more sense to use at least uint256 for everything rather than uint64, no?

@temptemp3
Copy link
Contributor Author

If one of the goals is to mirror ERC20 to presumably allow for seamless bridging between the two chains, it would make more sense to use at least uint256 for everything rather than uint64, no?

If one of the goals is to mirror the ERC20 standard for seamless bridging between the two chains, it does raise the question of whether using uint256 for all fields would be more appropriate than uint64.

While aligning with ERC20 would provide a high level of compatibility, we must consider the trade-offs involved. Using uint256 for all fields would significantly increase the cost of global storage and boxes. This is an important factor to consider, as it could affect the overall cost and adoption of the token.

However, it is indeed possible to create a smart contract token of this type using uint256. By replacing uint64 with uint256 and ensuring that the decimals value remains within the range of 256, you could achieve alignment with ERC20.

At present, sticking with uint64 seems appropriate, considering the goals and requirements of the project. However, if there is a strong demand or a specific use case that necessitates the use of uint256, it might be worth considering submitting a separate proposal that outlines the alternative token specification using uint256.

@joe-p
Copy link
Contributor

joe-p commented Jul 5, 2023

Using uint256 for all fields would significantly increase the cost of global storage and boxes. This is an important factor to consider, as it could affect the overall cost and adoption of the token.

There is no reason that the contract would need to store the full 32 bytes if not all bytes are used. Truncating the bytes for box storage but encoding as uint256 for arg/return values seems like a reasonable solution. The 32 bytes are there if you need them, but not a requirement. You can use uint64 if you want to save on opcodes and storage. It's entirely the developers choice.

If the developer really wanted to use global storage, it is a 0.02 ALGO extra cost over uint64, but not sure why you'd use global storage when box storage is cheaper.

However, it is indeed possible to create a smart contract token of this type using uint256. By replacing uint64 with uint256 and ensuring that the decimals value remains within the range of 256, you could achieve alignment with ERC20.

A contract that wants to use uint64 integers can follow a standard that uses uint256 encoding, but not the other way around. Having one standard that supports both use cases seems better than having two different standards.

@pbennett
Copy link
Contributor

pbennett commented Jul 5, 2023

If you want compatibility with ERC20, uint256 will be kind of a must. The issues with bridging ETH tokens over to ASAs and decimals has been an issue. PEPE has 18 decimals for eg. Its supply with those decimals isn't representable in a uint64 - in fact if brought over with that many decimals it'd only have about $100K maximum mcap based on an earlier price of PEPE.

@temptemp3
Copy link
Contributor Author

@joe-p @temptemp3
Thank you for the feedback! I appreciate your openness to reassess the approach. If there is a consensus and everyone agrees, updating the specification to use uint256 where applicable could be a beneficial decision. By aligning with uint256, you would ensure a higher level of compatibility with existing standards and enhance interoperability between chains.

@joe-p
Copy link
Contributor

joe-p commented Jul 5, 2023

Can you provide some more rationale to the timestamp event arguments? What is this expected to be? The timestamp when the client sends the transaction or the time of the last block? If one is parsing the logs to read events, it would be trivial to implicitly get the time of the block, no?

@temptemp3
Copy link
Contributor Author

temptemp3 commented Jul 5, 2023

@joe-p

Can you provide some more rationale to the timestamp even arguments? What is this expected to be? The timestamp the client sends the transaction or the time of the last block? If one is parsing the logs to read events, it would be trivial to implicitly get the time of the block, no?

The rationale to adding the timestamp event argument is that currently the library that I use to replay events does not include round time. So my workaround was to include the round time at confirmation as an argument of the event.

What is this expected to be?

It is to be expected as the round time at confirmation.

The timestamp the client sends the transaction or the time of the last block?

The timestamp of last block before confirmation.

If one is parsing the logs to read events, it would be trivial to implicitly get the time of the block, no?

Yes, I may be able to do something while parsing the logs to figure the time of the block.

With that said it may be viable to remove the timestamp and recover it elsewhere.

@temptemp3
Copy link
Contributor Author

After consideration, I have updated the spec draft to use uint256 and not include a timestamp argument in events. Now the spec is identical to ERC20.

@joe-p
Copy link
Contributor

joe-p commented Jul 21, 2023

What the rationale for adding the _ after each arg?

@temptemp3
Copy link
Contributor Author

temptemp3 commented Jul 21, 2023 via email

Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

I think this is an important standard. I hope my comments don't seem to nitpicky.

ARCs/arc-0200.md Outdated
A `Transfer` event SHOULD be emitted, with `from_` being the zero address, when a token is first minted.
A `Transfer` event SHOULD be emitted, with `to_` being the zero address, when a token is destroyed.

The `Approval` event MUST be emitted when the `approve` method is called successfully.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to "fix" the Ethereum misfeature here, and put increaseAllowance and decreaseAllowance into the standard?
https://www.adrianhetman.com/unboxing-erc20-approve-issues/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may want to fix this issue from the get-go because the ARC-200 spec implementation may easily be ported to a EVM based chain with this fix already applied. I haven't heard of any issues with front running on Algorand yet but this seems like a great way to future-proof the spec. Will be considering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jannotti I am on the side of not adding increaseAllowance and decreaseAllowance to the standard for simplicity sakes. I do think that implementations may add non-standard methods similar to the way OpenZeppelin does it with ERC-20 (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol)

I have a working ARC200 implementation that methods like increaseAllowance and decreaseAllowance for convenience.

ARCs/arc-0200.md Outdated Show resolved Hide resolved
ARCs/arc-0200.md Outdated

Ownership of a token by a zero address indicates that a token is out of circulation indefinitely, or otherwise burned or destroyed.

The methods `transfer` and `transferFrom` method MUST error when the balance of `from_` is insufficient.
Copy link
Contributor

Choose a reason for hiding this comment

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

transfer has no from_ argument. (I get your point, but the language needs to be changed.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here from is implied as the signer of the transaction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See also
a9174bc

ARCs/arc-0200.md Outdated
Ownership of a token by a zero address indicates that a token is out of circulation indefinitely, or otherwise burned or destroyed.

The methods `transfer` and `transferFrom` method MUST error when the balance of `from_` is insufficient.
The `transferFrom` method MUST error unless called by an approved spender as defined by an extension defined in this ARC.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this say "approved spender as defined by an extension defined in this ARC"? I think you're just referring to the approve system, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the approve system allows for multiple operators to spend on the behalf of other account if approved. I supposed we can revise it as follows.

approved spender -> spender

This may be more understandable since it matches the usage in the surrounding language and ABI spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revision here
a9174bc

Reworded as spender approved by owner instead of approved spender.
Added note that from is owner in case of transfer method

ARCs/arc-0200.md Outdated Show resolved Hide resolved
ARCs/arc-0200.md Outdated Show resolved Hide resolved
ARCs/arc-0200.md Outdated
The `Approval` event MUST be emitted when the `approve` method is called successfully.

A value of zero for the `approve` method and the `Approval` event indicates no approval.
When a `Transfer` event emits following the `transferFrom` method, this may also indicate that the approved value for the token is decremented.
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this should be stronger. The transferFrom method MUST decrement the current approval value for the from address of the to address's tokens.

I don't know if you want to require that a new Approval event is emitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one could be revised to be more clear.

The approval event must be emitted in case of approve and transferFrom

transferFrom emits both Approval and Transfer

approve emits only Approval

Going to come back to this one. I see your point. Thx!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded here
125f234

ARCs/arc-0200.md Show resolved Hide resolved
ARCs/arc-0200.md Outdated Show resolved Hide resolved
ARCs/arc-0200.md Show resolved Hide resolved
simple is better

Co-authored-by: John Jannotti <jannotti@gmail.com>
@temptemp3
Copy link
Contributor Author

I think this is an important standard. I hope my comments don't seem to nitpicky.

Not one bit. I appreciate your feedback and attention to detail.

temptemp3 and others added 2 commits July 21, 2023 11:58
Co-authored-by: John Jannotti <jannotti@gmail.com>
Nice

Co-authored-by: John Jannotti <jannotti@gmail.com>
@temptemp3
Copy link
Contributor Author

@jannotti Thank you for making a thorough pass through. I appreciate it!

@jannotti
Copy link
Contributor

jannotti commented Jul 28, 2023

I have proposed a new arc in #230 that standardizes app reference discoverability. It will essentially be deprecated once we have unnamed resources with simulate, but it seems like a good intermediate solution to me.

I love it. I have been thinking along similar lines. I'll comment there on some additional things this might do.

To give people an idea of what it does, if you want to explain what boxes balance(addr) takes, you implement arcXXX_balance(addr) and just write a couple lines of code to return the box names. So callers can know how to pack the real balance call.

Copy link
Collaborator

@SudoWeezy SudoWeezy left a comment

Choose a reason for hiding this comment

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

To be consistent with #219
Please accept suggested changes

ARCs/arc-0200.md Outdated Show resolved Hide resolved
ARCs/arc-0200.md Outdated Show resolved Hide resolved
ARCs/arc-0200.md Outdated Show resolved Hide resolved
ARCs/arc-0200.md Outdated Show resolved Hide resolved
ARCs/arc-0200.md Outdated Show resolved Hide resolved
ARCs/arc-0200.md Outdated Show resolved Hide resolved
ARCs/arc-0200.md Outdated Show resolved Hide resolved
ARCs/arc-0200.md Outdated Show resolved Hide resolved
ARCs/arc-0200.md Outdated Show resolved Hide resolved
ARCs/arc-0200.md Outdated Show resolved Hide resolved
@BunsanMuchi
Copy link

Some of the pushback I've seen in the community has been w/r to opt-ins. How anyone can airdrop an ERC-20, whereas for an ASA the user has to explicitly state they're willing to accept the asset. The airdropping could be annoying, a baiting/phishing attempt or potentially create an unforeseen tax-liability.

Is there any way we could improve on this by following the opt-in ethos?

I understand if it's seen as unimportant due to keeping consistency with ERCs but figured it'd be worth considering it.

@jannotti
Copy link
Contributor

Is there any way we could improve on this by following the opt-in ethos?

I understand if it's seen as unimportant due to keeping consistency with ERCs but figured it'd be worth considering it.

I'm firmly in favor of allowing airdrops without opt-in for these assets. It's not clear to me what the legal ramifications of airdropping tokens in Ethereum is. Have you really received property just because someone wrote down that you own it? I'm also not sure what the UI/UX should be for wallets when someone could be "spammed" by airdrops.

But I do know that, in both cases, the entire crypto ecosystem is getting along in the context of a model with no opt-ins. The fact that Algorand is different has caused more than one project to throw up their hands and decide they don't want to have to settle those difficult questions in a new context.

So I think there's room for a token standard that operates the way "everyone else" is doing it.

@BunsanMuchi
Copy link

BunsanMuchi commented Jul 31, 2023

Yeah, I understand that it's not really clear. I do feel sending a message of approval can act as an acceptance of ownership, but then again an opt-in isn't really that and there's just no real standard for this.

Maybe the answer is more Off-Chain, for example on other chains you tend to add a token to your wallet to have it reflected in your UI, and that's good enough, it should avoid most of the baiting/phishing attempts. Plus, since I imagine indexing balances is going to be a pain for most wallets I feel they'll end up doing something like this and avoid the other route of displaying all balances of ARC-200 due to the overhead.

I agree with the your assessment of there being room for a standard to be as close as "what everyone else is doing" as possible, so it's fair to just dismiss this.

temptemp3 and others added 19 commits August 5, 2023 13:04
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Co-authored-by: Stéphane <stephane@algorand.foundation>
Copy link
Contributor Author

@temptemp3 temptemp3 left a comment

Choose a reason for hiding this comment

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

Accept suggested changes

@github-actions github-actions bot removed the s-draft label Aug 8, 2023
@SudoWeezy SudoWeezy merged commit dcb1165 into algorandfoundation:main Sep 4, 2023
5 checks passed
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.

None yet

9 participants