Pandora-Labs-Org / erc404

396 stars 185 forks source link

No `Transfer` event emitted in new ERC404 implementation / unresolved ERC20 <> ERC721 spec collision #20

Closed fastackl closed 8 months ago

fastackl commented 8 months ago

The Transfer event was emitted in the legacy version ==> https://github.com/Pandora-Labs-Org/erc404-legacy/blob/master/src/ERC404.sol#L80-L84

But it is not emitted in the latest version

This event is an ERC-20 and ERC-721 standard event so it is likely to cause issues with platforms like OpenSea and Etherscan that expect this event to fire on transfer

You can see this issue manifesting on Etherscan here ==> https://etherscan.io/tx/0xb7e3fbc0f77cc31fe0536e2456dea7cc6ee0c5cf2db46276b6e98457ac7d5311

image

Closely related to this is that there is a collision between ERC20 and ERC721 in the Transfer event because they both are supposed to emit it

The solution that Pandora seems to have come to in its deployed version is that only transfers that trigger an ERC721 transfer emit the Transfer event e.g. ==> https://etherscan.io/tx/0xc1c0bdd6169338d02b8ab393620f5b62bc2dc0419bb53e81b75c41d6225dc5ad

In other words, Transfer events are only emitted for ERC-721

This means for example for this holder, etherscan can "see" the ERC-721 transfers: https://etherscan.io/address/0x8febbf7c0b133f746f9303deff1191334d1a0ae5#nfttransfers

But it can't "see" the ERC-20 transfers: https://etherscan.io/address/0x8febbf7c0b133f746f9303deff1191334d1a0ae5#tokentxns

mathdroid commented 8 months ago

Again, this code is not production ready. We’re weighing the decision of using granular, good events and having infra providers integrate (in the works), or we choose to support events for a certain standard (ERC721 since infra relies on these events for balance indexing).

We’ll almost definitely have an interim solution that emits standard events as we work to bring infra providers up to speed. The likely resolution is to adjust ERC721xxx events to just be their common naming, if you want to play around on testnet or something.

from @0xacme in tg

mathdroid commented 8 months ago

link #4

fastackl commented 8 months ago

One solution to the collision might be to have two Transfer events with different signatures

// Event for ERC-20 transfers
event Transfer(address indexed from, address indexed to, uint256 value);

// Event for ERC-721 transfers
event Transfer(address indexed from, address indexed to, uint256 indexed tokenId);

Then emit the appropriate one e.g. if transfer doesn't trigger ERC-721 then:

 // Emit the ERC-20 Transfer event
    emit Transfer(msg.sender, to, amount);

or if it does trigger ERC-721 transfer then:

// Emit the ERC-721 Transfer event
    emit Transfer(msg.sender, to, tokenId);

not 100% sure it would work in ERC404 context but just a thought

0xacme commented 8 months ago

One solution to the collision might be to have two Transfer events with different signatures

// Event for ERC-20 transfers
event Transfer(address indexed from, address indexed to, uint256 value);

// Event for ERC-721 transfers
event Transfer(address indexed from, address indexed to, uint256 indexed tokenId);

Then emit the appropriate one e.g. if transfer doesn't trigger ERC-721 then:

 // Emit the ERC-20 Transfer event
    emit Transfer(msg.sender, to, amount);

or if it does trigger ERC-721 transfer then:

// Emit the ERC-721 Transfer event
    emit Transfer(msg.sender, to, tokenId);

not 100% sure it would work in ERC404 context but just a thought

Those events unfortunately have the same signature. We'll support ERC721 events by default, but looking to move away from this as infra integrates.

fastackl commented 8 months ago

got it thanks yeah I just tested it out right now sorry was too lazy before

the indexed in the ERC721 Transfer event I suggested doesn't create a distinction in the signatures unfort