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

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

Closed
3s-jorgematos opened this issue Nov 23, 2023 · 4 comments · Fixed by #224
Closed

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

3s-jorgematos opened this issue Nov 23, 2023 · 4 comments · Fixed by #224
Labels
bug 🐛 Something isn't working p1 🟠 Indicates high priority item starter 🏁 Indicates low difficulty item

Comments

@3s-jorgematos
Copy link

🐛 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

  • Rust version: N/A
  • Operating System & Version: Debian latest docker image
  • Other relevant environment details: ERA_NODE_VERSION="v0.1.0-alpha.10"

📋 Additional Context

N/A

📎 Log Output

N/A

@3s-jorgematos 3s-jorgematos changed the title Log removed property is null and should be either true or false Log removed property is null and should be either true or false Nov 23, 2023
@3s-jorgematos
Copy link
Author

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
Copy link
Collaborator

Interesting, this should be a quick fix 👍

@MexicanAce MexicanAce added bug 🐛 Something isn't working p1 🟠 Indicates high priority item starter 🏁 Indicates low difficulty item labels Nov 25, 2023
@MexicanAce
Copy link
Collaborator

MexicanAce commented Nov 25, 2023

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working p1 🟠 Indicates high priority item starter 🏁 Indicates low difficulty item
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants