trufflesuite / truffle

:warning: The Truffle Suite is being sunset. For information on ongoing support, migration options and FAQs, visit the Consensys blog. Thank you for all the support over the years.
https://consensys.io/blog/consensys-announces-the-sunset-of-truffle-and-ganache-and-new-hardhat?utm_source=github&utm_medium=referral&utm_campaign=2023_Sep_truffle-sunset-2023_announcement_
MIT License
14.02k stars 2.31k forks source link

Cannot call once event handler multiple times for same event (v5) #1217

Open Maushundb opened 6 years ago

Maushundb commented 6 years ago

Issue

When trying to create a utility to wrap truffle contract events in a promise, it seems that adding consecutive "once" listeners causes only the first callback to fire, and the second never triggers.

Steps to Reproduce

const waitForEvent = async (contract, ...) => {
  return new Promise(async (resolve, reject) => {
    contract.MyEvent().once('data', event => {
      resolve(event.returnValues);
    });
    await contract.methodThatTriggersEvent(...);
  });
};
...
const contractInstance = await artifacts.require('Contract').new()
await waitForEvent(contractInstance) // resolves
await waitForEvent(contractInstance) // hangs and never resolves

A workaround I've used is using the web3 event directly, which works fine:

const waitForEvent = async (contract, ...) =>
  new Promise(async (resolve, reject) => {
    contract.contract.once('Transfer', (error, { returnValues }) => resolve(returnValues));
    await contract.methodThatTriggersEvent(...);
  });

The similar behavior seems to be happening with on / off - on / addListener adds the listener, but off / removeListener never actually removes the callback as intended and the second callback never gets added. Thus, it calls the first callback twice.

Expected Behavior

I expect the second promise to also resolve

Actual Results

Second call hangs

Environment

cgewecke commented 6 years ago

Hi @Maushundb Looked into this and agree that its seems like there's a problem repeatedly registering listeners for once to the same event. Thanks for your workaround. Was also able to get the expected result using on after reworking waitForEvent slightly.

It looks like the hanging you're seeing is caused by the way the promises are written in waitForEvent. . . on ganache at least, the event handler is firing/resolving before the promise to the event triggering transaction resolves.

The similar behavior seems to be happening with on / off - on / addListener adds the listener, but off / removeListener never actually removes the callback as intended and the second callback never gets added. Thus, it calls the first callback twice.

I couldn't reproduce this - do you have an example?

I'm going to edit title slightly - my take-away from this is

Maushundb commented 6 years ago

I couldn't reproduce this - do you have an example?

Something along the lines of:

const waitForEvent = async (contract, ...) => {
  return new Promise(async (resolve, reject) => {
    const id = Math.random();
    const cb = event => {
      console.log(id);
      contract.MyEvent().off('data', cb);
      resolve(event.returnValues);

    }
    contract.MyEvent().on('data', cb);
    await contract.methodThatTriggersEvent(...);
  });
};
...
const contractInstance = await artifacts.require('Contract').new()
await waitForEvent(contractInstance) // resolves, print random number
await waitForEvent(contractInstance) // hangs and never resolves, previous listener gets called twice, printing the same random number as above ^^

Thanks for the quick turnaround @cgewecke !

cgewecke commented 6 years ago

Mmm. I think the promise construction in waitForEvent might be problematic. There's a mix of async and 'resolve in a callback' there that's not mapping to the sequence things occur in.

Maushundb commented 6 years ago

Yeah thats the idea of how to turn a callback into an async function - wrap it in a promise and return that to be awaited on. The idea is that I want to not await the method, but rather the event the method will eventually trigger. In that case, the sequence is method -> event -> promise resolves, which is what that is supposed to abstract.

For a little more context, the method is triggering an oraclize call, which will eventually call a callback in my contract, which then emits the event I'm listening for. Thus I can't await on the method to finish, since the entire flow is async with the additional of oraclize. Thus await contract.methodThatTriggersEvent(...) will always resolve before the event fires. Hopefully that helps!!

cgewecke commented 6 years ago

Ah yes I get the promisify event part - the oraclize part has been abstracted away in your example in a way that is not reproducible. . . could you show in more detail what that actually looks like?

Maushundb commented 6 years ago

Sure! The contract looks something like:

function methodThatTriggersEvent() public {
  oraclize_query("URL", "http://my.url");
}

function __callback(bytes id, string result, bytes proof) public {
  emit MyEvent();
}

So I call methodThatTriggersEvent, which triggers oraclize to go fetch some stuff, then oraclize calls __callback, which eventually emits the event I'm listening for.