matter-labs / era-test-node

In-memory node that can be used for integration testing and debugging.
https://matter-labs.github.io/era-test-node
Apache License 2.0
308 stars 75 forks source link

Log `removed` property is null and should be either true or false #222

Closed 3s-jorgematos closed 11 months ago

3s-jorgematos commented 11 months ago

🐛 Bug Report for zkSync Era In-Memory Node

📝 Description

Log removed property is null and should be either true or false.

This is currently an issue for me because we are using NEthereum and trying to integrate with ZkSync Era, but the library fails to parse the null value into a boolean.

For example, when using Anvil this does not happen because the removed property is always either true or false and the library works as expected

Example of the JSON RPC response with this issue:

{
  "address": "0x000000000000000000000000000000000000800a",
  "blockHash": "0xa61653a7e06618db42e27012d818d9a1b1257513c7e09ae1cf16072400cef45c",
  "blockNumber": "0x29",
  "data": "0x00000000000000000000000000000000000000000000000002a84cd6cc724800",
  "l1BatchNumber": "0x15",
  "logIndex": "0x0",
  "logType": null,
  "removed": null,
  "topics": [
    "0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef",
    "0x00000000000000000000000036615cf349d7f6344891b1e7ca7c72883f5dc049",
    "0x0000000000000000000000000000000000000000000000000000000000008001"
  ],
  "transactionHash": "0x4ebd5fb10af384cf7f751d4a79a367bd5de29684f739c0e3724a0c246a576558",
  "transactionIndex": "0x0",
  "transactionLogIndex": "0x0"
}

🔄 Reproduction Steps

  1. Set up a smart contract with any event (dummy, just for testing)
  2. Call the contract in order for the event to be fired
  3. Get the event log (either with JSON RPC or a library)

🤔 Expected Behavior

Event log removed property should either be true or false, never null

😯 Current Behavior

Event log removed is null

🖥️ Environment

📋 Additional Context

N/A

📎 Log Output

N/A

3s-jorgematos commented 11 months ago

I think this oversight was not detected so far because with JS libraries, parsing null or false would end up with very similar behavior due to the nature of how JavaScript works.

In the case of NEthereum we are using C# and the type safety properties of this language are not compatible with this.

Also, would like to note that we also tried a tool called Eventeum (Java) and we are having problems with it processing events, but were not able to pinpoint it to this issue.

MexicanAce commented 11 months ago

Interesting, this should be a quick fix 👍

MexicanAce commented 11 months ago

Additional info: It looks like ethers has this field be nullable/optional, but will not serialize the value accordingly. As this value looks to always be set, this seems strange and for your C# implementation you should expect that null is a possibility (for future changes/implementations).

https://github.com/gakonst/ethers-rs/blob/8175f0f97070ad69abd5dfa7b8df31f1c1dcc3d6/ethers-core/src/types/log.rs#L60

For now, I'll set this to value to False

3s-jorgematos commented 11 months ago

Just wanted to add I've been testing with the public testnet and this issue does not happen with NEthereum. This reinforces the idea that the test node should change the behavior to be more consistent