ERC20 Approval event conformance

[k06a]

I found that OZ ERC20 implementation differs with your DAI implementation.

• OZ ERC20 emits Approval event on every approval change:

  • approve https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L217
  • transfer https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L63
  • transferFrom https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L100
  • _burnFrom https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L228

• DAI implementation emits Approval event only inside approve method:

  • approve https://github.com/makerdao/dss/blob/master/src/dai.sol#L100
  • transfer https://github.com/makerdao/dss/blob/master/src/dai.sol#L76
  • transferFrom https://github.com/makerdao/dss/blob/master/src/dai.sol#L76
  • burn https://github.com/makerdao/dss/blob/master/src/dai.sol#L92

ERC20 Token Standard tells:

Approval
MUST trigger on any successful call to approve(address _spender, uint256 _value).

For me it seems OZ implementation should be fixed to conform, or maybe they have some reasons to follow this behavior.

[k06a]

@nventuro provided a link to the discussion where they came up to the decision to emit Approval event on every allowance change: OpenZeppelin/openzeppelin-contracts#707

[MrChico]

If you want to reconstruct the allowance fron LOGS (which is not really recommended, as LOGS shouldnt be relied upon for historical data) you can easily recalculate it from the Approval + Tranfer events. I dont think we should add additional logs here.

For the record, permit also emits an Approval event

[k06a]

@MrChico main OZ point was that it is not possible to recover state based on Approval+Transfer events since account can transferFrom to different destination than its own address.

[MrChico]

Using logs to recover state is not a good practice. I'm not sure that one would want to condone such behaviour

[nventuro]

@MrChico is there any particular reason why you wouldn't rely on logs?

it is not possible to recover state based on Approval+Transfer events since account can transferFrom to different destination than its own address.

Recovering state is not the only issue: when using transferFrom with no Approval events, it is not possible to notice that the allowance has changed from just the event logs: you'd have to look inside the calldata, extract the function selector, and compare it against transfer and transferFrom's.

[MrChico]

That sounds like recovering state to me. Determining the allowance of a user is easy, you just fetch the corresponding allowance from state. Maybe you describe a use case where not having the Approval event in these functions would cause an issue?

[nventuro]

The two basic use cases for events are: a) create a timeline for state, and b) react to something. transferFrom modifies allowance without emitting an Approval event, so it is not possible to react to allowance changes using events alone.

[k06a]

@nventuro case b makes more sense for me.

[MicahZoltu]

For clarity, the thing that is a bad practice is using logs to recover ancient data like many dapps do. What logs are useful for is people who want to monitor/track changes live (which they then may store in an external database for historical lookup). In general, you log basically the same thing regardless of whether the user is just tracking head or trying to lookup ancient state. The thing that is a bad practice is relying on those logs being available forever.

[MrChico]

Thanks for everybody's comments. I dont have a strong feeling about this, and it seems like others do. So in the case of this contract one would add another event in the TransferFrom clause that actually diminishes the allowance? That would be L75 in dai.sol

[MicahZoltu]

One last feeble attempt at ERC777: I think the right solution here is to have DAI be an ERC777 token instead of ERC20. ERC777 is backward compatible with ERC20, but it fixes a lot of mistakes that were made in ERC20, including this one.

[k06a]

@MicahZoltu sorry, but I can't understand why anyone would prefer ERC777 instead of ERC20. For me, it seems like an overdeveloped solution with the not clear idea behind it. Additionally, operators behave like infinite approves, which are very dangerous. I'd better add to Ethereum ability to group multiple transactions in one to make atomic approve+call: ethereum/EIPs#2243

[MicahZoltu]

@k06a ERC-777 operators are not the same as ERC20 approvals, they are intended for "this address is allowed to operate on my tokens on my behalf" rather than "I want to transfer some tokens as part of a larger transaction". For the latter, you would use send(to, amount, calldata) so you can send some tokens and then follow-up with a contract call. For example, if you wanted to send DAI to a maker contract to close out some debt you would do so with send(cdp, amount, <calldata_for_debt_closure>).

Unlike ERC20, ERC777 went through a lot of rounds of feedback between a lot of people who have worked with ERC20 (and other) tokens in the past. Everything in it is incredibly deliberate (unlike ERC20) and serves a distinct purpose. If you think something in ERC777 doesn't make sense, I recommend reading through the discussion history for its development as I can almost guarantee that whatever concern you have was brought up and debated.

[MrChico]

Yeah. Not a fan of 777. Calls to unknown code goes strictly against the praxis of this codebase

[MicahZoltu]

@MrChico This would be at the edge and for ecosystem interoperability. Already vat.dai doesn't follow any standard, the DAI token contract exists explicitly for ecosystem integration and standardization. Basically everything about the DAI token contract goes against the praxis of this codebase because it is an integration point and it has intentionally been designed to live external to the rest of Maker so that it can go against the norms found throughout the maker codebase without having to put the rest of the Maker codebase at risk.