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

Event Log types #415

Closed
epheph opened this issue Feb 5, 2019 · 3 comments
Closed

Event Log types #415

epheph opened this issue Feb 5, 2019 · 3 comments
Labels
discussion Questions, feedback and general information.

Comments

@epheph
Copy link

epheph commented Feb 5, 2019

In abstract-provider, the type for a Log is defined as:

export interface Log {
    blockNumber?: number;
    blockHash?: string;
    transactionIndex?: number;

    removed?: boolean;

    transactionLogIndex?: number,

    address: string;
    data: string;

    topics: Array<string>;

    transactionHash?: string;
    logIndex?: number;
}

I see that many of these are marked as optional with a ? . Some, like transactionHash, seem like they would always have to be there and others, like blockHash should perhaps be nullable and not optional (only if you wanted to match the JSON-RPC API more closely).

It seems that there are two types of Logs that get returned, one from a getLogs of a mined transaction (including logIndex, blockHash, etc) and one from a getLogs of pending (or from eth_getFilterChanges?), which has Object keys that are nullable/undefined?

Does that sound right architecturally?

@ricmoo ricmoo added the discussion Questions, feedback and general information. label Feb 8, 2019
@ricmoo
Copy link
Member

ricmoo commented Feb 8, 2019

It might make more sense to break that into two types of logs, but there is one other reason sometimes those things aren't filled in nicely, mainly Ganache, which is not the most standards compliant beast, especially slightly older versions.

But for the most part a lot of those changes (as I recall) were added when the "pending" filter was added. I'll have to check my notes.

For now though, I think it makes enough sense though, no? Or are there problems with marking them as optional? I'm still fairly new to TypeScript.

@ricmoo
Copy link
Member

ricmoo commented Apr 4, 2020

I don't think that there are any Ganache issues any more with this, so I have made removed the optional component. If there are any problems please re-open the issue, but I think this is safe. :)

I also removed the transactionLogIndex, since that is only populated in certain calls (so it's not reliably available) and anyone who would have it, could easily compute it from the data the call returned.

The changes are local, but will go out with the next v5 release.

Thanks! :)

@ricmoo
Copy link
Member

ricmoo commented Apr 4, 2020

This was published in 5.0.0-beta.180.

Thanks! :)

michaeltout pushed a commit to michaeltout/ethers.js that referenced this issue Aug 23, 2020
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

2 participants