Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Add removed field to Log JSON #651

Merged
merged 2 commits into from
Oct 28, 2020
Merged

Conversation

iostat
Copy link
Contributor

@iostat iostat commented Oct 18, 2020

Fixes #336

This is a fairly trivial change, especially considering Ganache will never reorg and thus the removed property is moot (which reasonably explains its omission in the first place).

However, for those of us who care whether or not a log was reorged as part of their application logic, and aren't using JavaScript (i.e., care about well-formed JSON-RPC responses and can't use the truthiness of log.removed === undefined), this lack of removed renders Ganache unusable as a Web3 provider.

@davidmurdoch
Copy link
Member

davidmurdoch commented Oct 18, 2020

Looks good. I'd like to see a simple test to check the value of the field before merging though. Is this something you can do?

Aside: this field has also been added in a separate branch where we are rewriting ganache-core: https://github.com/trufflesuite/ganache-core/blob/4a081acfad686e2f94c49f6087df6bb9722d2d1d/src/chains/ethereum/src/things/blocklogs.ts#L137. We do plan on stimulating reorgs in the future.

@iostat
Copy link
Contributor Author

iostat commented Oct 18, 2020

Hope that was a reasonable place to put the test. I didn't want to add an entire transaction flow just to test the presence of a field, and the headline of test suggested it's appropriate anyway.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TX Receipts and getFilterChanges responses should return a removed property for each log.
2 participants