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

Question: Warnings raised when there are Events with the same name but different inputs? #905

Closed
yosriady opened this issue Jun 24, 2020 · 14 comments
Labels
discussion Questions, feedback and general information.

Comments

@yosriady
Copy link

yosriady commented Jun 24, 2020

On ethers 5.0.2

Given the following ABI with two Transfer events with different inputs:

{
  "abi": [
    {
      "anonymous": false,
      "inputs": [
        {
          "indexed": true,
          "name": "from",
          "type": "address"
        },
        {
          "indexed": true,
          "name": "to",
          "type": "address"
        },
        {
          "indexed": false,
          "name": "value",
          "type": "uint256"
        },
        {
          "indexed": false,
          "name": "data",
          "type": "bytes"
        }
      ],
      "name": "Transfer",
      "type": "event"
    },
    {
      "anonymous": false,
      "inputs": [
        {
          "indexed": true,
          "name": "from",
          "type": "address"
        },
        {
          "indexed": true,
          "name": "to",
          "type": "address"
        },
        {
          "indexed": false,
          "name": "value",
          "type": "uint256"
        }
      ],
      "name": "Transfer",
      "type": "event"
    }
  ]
}

In contract form this would look like:

contract Test {
    event Transfer(address indexed from, address indexed to, uint256 value);
    event Transfer(address indexed from, address indexed to, uint value, bytes data);
}

When calling the above contract, [email protected] logs the following warning:

Duplicate definition of Transfer 
(Transfer(address,address,uint256,bytes), 
  Transfer(address,address,uint256))

Source:

Object.keys(uniqueFilters).forEach((name) => {
const filters = uniqueFilters[name];
if (filters.length === 1) {
defineReadOnly(this.filters, name, this.filters[filters[0]]);
} else {
logger.warn(`Duplicate definition of ${ name } (${ filters.join(", ")})`);
}
});

Why do we warn users for having Events with duplicate names? I would expect that the intention is to check for duplicate Event signatures, for example:

  • [Transfer(address,address,uint256,bytes), Transfer(address,address,uint256)] is OK 👍 since they have different event signatures.
  • [Transfer(address,address,uint256,bytes), Transfer(address,address,uint256,bytes)] is NOT OK 👎
    because they have duplicate event signatures.

Is this because of the events API expecting an event name?

@yosriady yosriady changed the title Warnings raised when there are Events with the same name but different inputs Question: Warnings raised when there are Events with the same name but different inputs? Jul 2, 2020
@ricmoo
Copy link
Member

ricmoo commented Sep 7, 2020

The main reason is because of how you would access them. If you have a single Transfer event, then you could use contract.events.Transfer to get the Transfer fragment. But if there are multiple, you need to use contract.events["Transfer(address, address,uint256"] for example.

Many of these issues go away in v6 which will use a Proxy, but for now this is something there isn't a great way around. You can always lower the Logger logLevel to error if the console logs are too much an issue. :)

@ricmoo ricmoo added the discussion Questions, feedback and general information. label Sep 7, 2020
@ricmoo
Copy link
Member

ricmoo commented Oct 8, 2020

I think this has been answered, so I'm going to close it. If you still have further questions though, please feel free to re-open.

Thanks! :)

@ricmoo ricmoo closed this as completed Oct 8, 2020
@TheGreatHB
Copy link

TheGreatHB commented Aug 17, 2021

The main reason is because of how you would access them. If you have a single Transfer event, then you could use contract.events.Transfer to get the Transfer fragment. But if there are multiple, you need to use contract.events["Transfer(address, address,uint256"] for example.

Many of these issues go away in v6 which will use a Proxy, but for now this is something there isn't a great way around. You can always lower the Logger logLevel to error if the console logs are too much an issue. :)

Hi, ricmoo.
Would you let me know how to lower the Logger logLevel to error...?

Solved it with #379
thanks @ricmoo !

@lukehutch
Copy link

lukehutch commented May 28, 2022

The main reason is because of how you would access them. If you have a single Transfer event, then you could use contract.events.Transfer to get the Transfer fragment. But if there are multiple, you need to use contract.events["Transfer(address, address,uint256"] for example.

Many of these issues go away in v6 which will use a Proxy, but for now this is something there isn't a great way around. You can always lower the Logger logLevel to error if the console logs are too much an issue. :)

@ricmoo However the exact problem occurs with overloaded function names, and ethers does not report a warning when there are overloaded functions -- you have to just manually call overloaded functions using a dict lookup rather than accessing a Function-typed field of the Contract. So why are events treated differently? This warning is annoying because some standards do in fact emit multiple events with the same name but different parameter signatures.

@lukehutch
Copy link

@ricmoo Another problem with this warning is that the warning is issued even when a user is not subscribed to monitor the event in question.

@ricmoo
Copy link
Member

ricmoo commented May 29, 2022

@lukehutch Because v5 aims to be ES3 compatible, there is no useful way at runtime to issue that warning in a way that doesn’t result in uncallable methods with no feedback.

It does behave identically for both functions and events in v5; overloaded methods and events must be specified by their normalized signature, or the unnecessary fragments dropped from the ABI. I personally recommend dropping unnecessary fragments, as it makes the code far more readable, and shrinks the size of the app by removing descriptors that are never used (and cannot be pruned by tree-shaking).

That said, this is no longer an issue in v6, as it can use ES5 Proxy, which provides a runtime error if something is misused, and also the Typed API to make code far more readable in the case of overloaded features. :)

@lukehutch
Copy link

@ricmoo but I get this warning on ethers v5 even if I'm not listening to the Transfer event -- that was my point, there should be no warning if the caller doesn't even care about the event with the overloaded name.

@ricmoo
Copy link
Member

ricmoo commented May 29, 2022

@lukehutch But that cannot be done without Proxy. Because there is no way to detect it is being used. Imagine Transfer(address) and Transfer(uint); that means there is no Transfer added by its bare name, so without a warning, Transfer would just not work without any meaningful way to tell why. And there is no way to do anything different, whether Transfer is called or not called.

With ES3 features, there is no way to really do this if a way without breaking other parts and uses of the libraries (e.g. using deferred errors, which break enumeration).

@ricmoo
Copy link
Member

ricmoo commented May 29, 2022

I guess this is the question: how would ethers know whether you care about it or not? The best way would be to not include them in the ABI. But there isn’t much that can be done otherwise. (Proxy makes this all go away ;))

@lukehutch
Copy link

@ricmoo I don't understand what you mean about Proxy, sorry -- but I assume you're saying that once ethers v6 is released, I can just upgrade to that version and the warning will go away?

@ricmoo
Copy link
Member

ricmoo commented May 29, 2022

@lukehutch Without the ES5 Proxy, there is no way to detect whether something is used or not, which is why v5 must assume it is used.

Yes, there is no warning in v6, but it is still very beta. :)

@ricmoo
Copy link
Member

ricmoo commented May 29, 2022

@lukehutch
Copy link

Well could you please at least create a setting for disabling this warning on ES3? The user should be able to decide whether they care about it or not. I have hundreds of lines of this warning in my test output, and that obscures legitimate test output.

@sullof
Copy link

sullof commented Mar 17, 2023

@lukehutch
I solved it intercepting the call to console.log, adding:

const log = (msg) => {
  if (/Duplicate definition of/.test(msg)) {
    return;
  }
  console.log(msg);
}
console.log = log;

at the beginning of the helpers.js lib that I import in any test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Questions, feedback and general information.
Projects
None yet
Development

No branches or pull requests

5 participants