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

Add ERC: Temporary Approval Extension for ERC-20 #358

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

byshape
Copy link

@byshape byshape commented Apr 2, 2024

Among all cases of ERC-20 token transactions, a popular one is when smart contracts approve token spending to other contracts. Often tokens are approved for only one transaction.

Following the ERC-20 standard, if a smart contract wants to approve the spending of tokens to another smart contract for only one transaction, this causes the allowance saved in storage to be updated and retrieved.

Token allowances utilising EIP-1153 transient storage are a cheaper alternative to the regular storage allowances.

@eip-review-bot
Copy link
Collaborator

eip-review-bot commented Apr 2, 2024

File ERCS/erc-7674.md

Requires 1 more reviewers from @axic, @g11tech, @SamWilsn, @xinbenlv

byshape and others added 2 commits April 3, 2024 10:37
Co-authored-by: Andrew B Coathup <28278242+abcoathup@users.noreply.github.com>
@eip-review-bot eip-review-bot changed the title Add Transient Approval Extension for ERC-20 Add ERC: Transient Approval Extension for ERC-20 Apr 3, 2024
@github-actions github-actions bot added w-ci and removed w-ci labels Apr 3, 2024
@github-actions github-actions bot removed the w-ci label Apr 3, 2024
@ZumZoom
Copy link

ZumZoom commented Apr 3, 2024

Commit 85f7d71 removes EIP-1153 reference from the description section but it seems that EIP Walidator needs to be fixed instead:

https://github.com/ethereum/ERCs/actions/runs/8536634399/job/23385573486#step:3:14

Error: error[preamble-refs-description]: unable to read file ``erc-1153.md``: Io: JsValue(Error: ENOENT: no such file or directory, open 'ERCS/erc-1153.md'

It seems that it disallows referencing any EIP in the description section.

ERCS/erc-7674.md Outdated Show resolved Hide resolved
ERCS/erc-7674.md Outdated Show resolved Hide resolved
@Amxx
Copy link
Contributor

Amxx commented Apr 3, 2024

IMO "transient" is a technical name that refers to the underlying mechanism used here, but should not be exposed in the ABI or in the "docuementation" or this feature. I propose using "temporary allowance" instead of "transient allowance", as I believe this naming would be more user-friendly for people that have no knowledge of EIP-1153

@Amxx
Copy link
Contributor

Amxx commented Apr 3, 2024

This ERC should clearly mention that:

  • No event is required when setting a temporary/transient allowance
  • This ERC does not break ERC20's transferFrom, because setting a temporary/transient allowance falls in the scope of "explicitelly authorizing the transfer"

@eip-review-bot eip-review-bot changed the title Add ERC: Transient Approval Extension for ERC-20 Add ERC: Temporary Approval Extension for ERC-20 Apr 4, 2024
@byshape
Copy link
Author

byshape commented Apr 4, 2024

Mikhail @ZumZoom and I have collected some opinions on naming transient/temporary, with the majority leaning towards temporary as the friendlier option. So I replaced uses wherever it was appropriate.

ERCS/erc-7674.md Outdated Show resolved Hide resolved
ERCS/erc-7674.md Outdated Show resolved Hide resolved
@ernestognw
Copy link
Contributor

I left a review, thanks for pointing out @Amxx!

The ERC looks good, but since it's defining an interface for a temporary approval, I don't see why to restrict this to transient storage. While it makes sense, I think the benefits of this kind of approval can be achieved regardless of transient storage support in a context where using storage may not be a big deal (e.g. an L2)

I would suggest generalizing the interface and its semantics while keeping the suggestions for using transient storage if EIP-1153 is available, but I understand the intention of making a transient-approval ERC 😅

Copy link
Contributor

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

Left some comments in case you're open to generalize it. Feel free to disregard.

ERCS/erc-7674.md Outdated Show resolved Hide resolved
ERCS/erc-7674.md Outdated Show resolved Hide resolved
ERCS/erc-7674.md Outdated Show resolved Hide resolved
ERCS/erc-7674.md Outdated Show resolved Hide resolved
ERCS/erc-7674.md Outdated Show resolved Hide resolved
ERCS/erc-7674.md Outdated Show resolved Hide resolved
@byshape
Copy link
Author

byshape commented Apr 5, 2024

@ernestognw Thank you for your comments!

While it makes sense, I think the benefits of this kind of approval can be achieved regardless of transient storage support in a context where using storage may not be a big deal (e.g. an L2)

Do you already have any ideas on how this could be implemented?

@ernestognw
Copy link
Contributor

ernestognw commented Apr 5, 2024

A quick thought was another mapping with approvals per block number (or timestamp), in this way it's not needed to clean up the value at the end since it will just get invalidated immediately.

Here's a quick PoC

Co-authored-by: Ernesto García <ernestognw@gmail.com>
@byshape
Copy link
Author

byshape commented Apr 6, 2024

I prefer to keep the temporary storage for this ERC within a transaction rather than a block.
EIP-1153 already operates within these limits so that compatible chains will be able to use this functionality. The rest may try to implement this logic in any other non-conflicting way.

ERCS/erc-7674.md Outdated Show resolved Hide resolved
ERCS/erc-7674.md Outdated Show resolved Hide resolved
ERCS/erc-7674.md Outdated Show resolved Hide resolved
ERCS/erc-7674.md Outdated Show resolved Hide resolved
ERCS/erc-7674.md Outdated Show resolved Hide resolved
ERCS/erc-7674.md Outdated Show resolved Hide resolved
@Amxx
Copy link
Contributor

Amxx commented Jun 6, 2024

@byshape created byshape#1 to update this PR. We should stry to get it merge sonner rather than latter.

Amxx and others added 2 commits June 10, 2024 15:40
Co-authored-by: Mikhail Melnik <by.zumzoom@gmail.com>
Update ERC-7674 based on PR comments
@github-actions github-actions bot removed the w-ci label Jun 10, 2024
```solidity
function temporaryApprove(address spender, uint256 value) public returns (bool success)
```
Call to `temporaryApprove(spender, value)` allows `spender` to withdraw within the same transaction from `msg.sender` multiple times, up to the `value` amount. This temporary allowance is to be considered in addition to the normal (persistent) [ERC-20](./eip-20.md) allowance, the total value that spender is able to spend during the transaction is thus capped by the sum of the temporary and the normal (persistent) allowances.
Copy link
Contributor

Choose a reason for hiding this comment

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

This temporary allowance is to be considered in addition to the normal (persistent) ERC-20 allowance

This forces an SLOAD of the current allowance, resulting in higher costs than one would want from this feature. I think it should be a temporary allowance override instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree.

If the temporary allowance takes priority (this should be mentionned somewhere if it isn't already), and if the temporary allowance is sufficient to cover the need of some function that consumes allowance (transferFrom), then the allowance can be consumed from the temporary part without having to go consume anything in the persistent part.

You only need to lean (and update) the persistent part if the temporary part is not sufficient.

An analogy would be:

  • you want to cook rice,
  • you have 2kg of rice in your kitchen (fast access - eq to temporary)
  • you have 50kg of rice stored in you basement (slow access - eq to persistent)

it is true that:

  • you have 52kg available
  • to measure how much you have available, you have to check both the kitchen and the basement
  • if you need to cook diner, and that diner requires 500g of rice, the kitchen is sufficient, and you don't need to go to the basement. Rice in the basement is available if you need, but you don't need to access it unless there isn't enough in the kitchen for what you are planning to cook.

Copy link
Contributor

@Amxx Amxx Jun 10, 2024

Choose a reason for hiding this comment

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

What do you think of

Suggested change
Call to `temporaryApprove(spender, value)` allows `spender` to withdraw within the same transaction from `msg.sender` multiple times, up to the `value` amount. This temporary allowance is to be considered in addition to the normal (persistent) [ERC-20](./eip-20.md) allowance, the total value that spender is able to spend during the transaction is thus capped by the sum of the temporary and the normal (persistent) allowances.
Call to `temporaryApprove(spender, value)` allows `spender` to withdraw within the same transaction from `msg.sender` multiple times, up to the `value` amount. This temporary allowance is to be considered in addition to the normal (persistent) [ERC-20](./eip-20.md) allowance, the total value that spender is able to spend during the transaction is thus capped by the sum of the temporary and the normal (persistent) allowances. While it SHOULD be possible for a `transferFrom` operation to consume both types of allowance, the consumption of the temporary allowance SHOULD take priority over the consumption of the persistent allowance. Therefore, if the temporary allowance is sufficient for executing a `transferFrom` operation, the persistent allowance SHOULD not be loaded/updated from the storage. Consumption of persistent allowance, which implies storage accesses, SHOULD be performed only if the temporary allowance is not sufficient for the operation being executed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, my confusion was because of the effect on the allowance() function. If the allowances have to be added, this forces allowance to read from both. However, in the context of transferFrom it should not be necessary to do this, as you've described.

I am not sure if the effect on allowance is something that we should care about.

---
eip: 7674
title: Temporary Approval Extension for ERC-20
description: An interface for ephemeral ERC-20 approvals
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: An interface for ephemeral ERC-20 approvals
description: An interface for ERC-20 approvals lasting a single transaction


## Motivation

User are often require to set token approval that will only be used once. It is common to leave unexpected approvals after these interactions. [EIP-1153](./eip-1153.md) introduces a cheap way to handle temporarily allowances.
Copy link
Collaborator

Choose a reason for hiding this comment

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

will only be used once

This is a bit misleading. This approval can be used multiple times, as long as it's in a single transaction.


Compliant contracts MUST add a temporary allowance to the permanent one when returning the allowed amount to spend in the `allowance` function. In case the sum of the temporary and permanent allowance overflow, `type(uint256).max` MUST be returned.

No event is required when setting a temporary allowance, but compliant contracts MAY emit a specific event.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should specify the event, or clarify what "specific" means here. The easiest would be to define the event, but leave it optional.

```
Call to `temporaryApprove(spender, value)` allows `spender` to withdraw within the same transaction from `msg.sender` multiple times, up to the `value` amount. This temporary allowance is to be considered in addition to the normal (persistent) [ERC-20](./eip-20.md) allowance, the total value that spender is able to spend during the transaction is thus capped by the sum of the temporary and the normal (persistent) allowances.

The storage for the temporary allowances MUST be different to that of the regular allowance. Compliant contracts MAY use the transient storage [EIP-1153](./eip-1153.md) to keep the temporary allowance. For each `owner` and `spender`, the slot MUST be uniquely selected to avoid slot collision. Each slot index SHOULD be derived from the base slot index for temporary allowances, `owner` and `spender` addresses. Slot MAY be derived as `keccak256(spender . keccak256(owner . p))` where `.` is concatenation and `p` is `keccak256` from the string uniquely defining temporary allowances in the namespace of the implementing contract.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is all still implementation detail. You should only specify the externally visible behaviour in the Specification section. You can put recommendations in the Reference Implementation section. How about:

Suggested change
The storage for the temporary allowances MUST be different to that of the regular allowance. Compliant contracts MAY use the transient storage [EIP-1153](./eip-1153.md) to keep the temporary allowance. For each `owner` and `spender`, the slot MUST be uniquely selected to avoid slot collision. Each slot index SHOULD be derived from the base slot index for temporary allowances, `owner` and `spender` addresses. Slot MAY be derived as `keccak256(spender . keccak256(owner . p))` where `.` is concatenation and `p` is `keccak256` from the string uniquely defining temporary allowances in the namespace of the implementing contract.
Each temporary allowance MUST persist until the end of the transaction that created it (unless overwritten by another call to `temporaryApprove`.) Each temporary allowance MUST be cleared at the end of the transaction that created it. See [Using Transient Storage](#using-transient-storage) for an example.

Then moving this content to the Reference Implementation section under a "Using Transient Storage" heading.

Copy link
Contributor

@Amxx Amxx Jun 11, 2024

Choose a reason for hiding this comment

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

(unless overwritten by another call to temporaryApprove or consumed by a call to transferFrom).

ERCS/erc-7674.md Show resolved Hide resolved
ERCS/erc-7674.md Show resolved Hide resolved
ERCS/erc-7674.md Show resolved Hide resolved
ERCS/erc-7674.md Show resolved Hide resolved
ERCS/erc-7674.md Show resolved Hide resolved
@github-actions github-actions bot added the w-ci label Jul 15, 2024
Copy link

The commit 8a11a87 (as a parent of e167942) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot removed the w-ci label Aug 8, 2024
@SamWilsn
Copy link
Collaborator

SamWilsn commented Aug 8, 2024

I took the liberty of fixing your EIP/ERC links. You do need to use [ERC-20](./eip-20.md) when linking to ERCs because of a quirk in our rendering system.

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.

8 participants