trufflesuite / ganache-cli-archive

Fast Ethereum RPC client for testing and development. See https://github.com/trufflesuite/ganache for current development.
https://www.trufflesuite.com/ganache
MIT License
3.36k stars 697 forks source link

Filtering with checksum addresses #494

Closed carver closed 6 years ago

carver commented 6 years ago

Expected Behavior

Current Behavior

Everything as expected, except the last step: no logs are returned.

Note that if the lower-case address is used in the filter instead, that logs are returned. So all other steps appear to be configured correctly.

Possible Solution

Force addresses to lower-case in the filter before searching? (optionally, verifying checksums)

Steps to Reproduce (for bugs)

I'm cross-posting from a web3py bug report, which includes source: https://github.com/ethereum/web3.py/issues/674

Given the simplicity of the workaround, it seems that the problem should be easy to reproduce.

Context

Without this fix, ganache filters will appear broken in the next stable release of web3py, which is likely within a week or two. All other nodes appear to work as expected.

Your Environment

I'll ask the original posters on the web3.py issue to fill in this info:

jordanjambazov commented 6 years ago

I confirm the issue was reproducible 24 days ago with latest version of Docker Ganache-CLI: v6.1.0-beta.2

pipermerriam commented 6 years ago

Ping, any chance this can be fixed soon? We get a steady stream of people encountering this error over in https://github.com/ethereum/web3.py

5hanth commented 6 years ago

"Force addresses to lower-case in the filter before searching" is accepted solution to fix this for now?

boolafish commented 6 years ago

Ping! we would really like to see this solved as well

lbrth commented 6 years ago

Does anyone know when the issue will be solved ?

gakonst commented 6 years ago

+1 ping, would love if this was solved!

llxxee commented 6 years ago

exact same issue with Ganache CLI v6.0.3 (ganache-core: 2.0.2)

5hanth commented 6 years ago

I can give a PR if I may know what excatly is the agreed solution to fix this. Force lowercase addresses?

lbrth commented 6 years ago

@5hanth Force lowercase addresses works for me but by default with the current version of web3.py you will raise an exception : InvalidAddress("Address has an invalid EIP checksum", value). To fix that we have to edit the current version of web3.py, or get correct patch. Please see the issue #826 (by the way, thank's to @gakonst) to see how it's work and how to fix.

carver commented 6 years ago

I can give a PR if I may know what excatly is the agreed solution to fix this. Force lowercase addresses?

Yes, since it seems that ganache expects lower-case hex data internally, then it should force all incoming hex to lower-case (addresses, filter parameters, block hashes, etc)

To fix that we have to edit the current version of web3.py, or get correct patch.

json-rpc clients should be able to send hex-encoded binary in any case: upper, lower, or mixed. So changing how web3.py works is not a good solution here. (Even if web3.py forced all hex to lower case, then the same change would have to happen in all other clients, and even hand-written json-rpc calls)

robinagist commented 6 years ago

same issue here. straightout rejection by web3.py on addresses.

edevil commented 6 years ago

Sad that web3.py and Ganache cannot interoperate correctly. They were my goto tools for ethereum development.

benjamincburns commented 6 years ago

I'll do my best to get this one fixed & released in the next week or two.

benjamincburns commented 6 years ago

Root cause found, see MetaMask/provider-engine#251. In spite of the issue being in a MetaMask repo, I'm likely to blame for this, I think I broke it in the subscription subprovider refactoring.

benjamincburns commented 6 years ago

PR submitted: MetaMask/provider-engine#252

benjamincburns commented 6 years ago

@carver would you please take a quick look at the test I submitted in that PR and to make sure that it exercises the problem you're experiencing? Just want to make sure that I haven't misunderstood this issue. I'm fairly confident that I have it right, but I'd hate to have to push out two releases to get this fixed :-)

edevil commented 6 years ago

@benjamincburns It seems correct. Is there a way to quickly build ganache-cli with the current master of ganache-core to try it?

carver commented 6 years ago

I'm not familiar enough with js testing to be 100% sure, but it looks roughly right :+1:. Thanks @benjamincburns !

benjamincburns commented 6 years ago

The tests in that project could certainly be made a bit more readable.

I had to shift priorities today so no release until Monday. Sorry for the delay.

benjamincburns commented 6 years ago

@carver & @pipermerriam I've just published ganache-cli@v6.1.2 - this should contain this fix. Please let me know if you're still having troubles!

edevil commented 6 years ago

@benjamincburns Shouldn't this have triggered a new build at https://hub.docker.com/r/trufflesuite/ganache-cli/tags/ ?

mikeseese commented 6 years ago

@edevil looks like the build is now there on docker

edevil commented 6 years ago

@seesemichaelj Thanks. However, it seems the original issue was not fixed. @benjamincburns

mikeseese commented 6 years ago

We'll take a look at this when we get a chance, thanks for letting us know!

mikeseese commented 6 years ago

If I'm correct, #127 should fix this! Going to close this, should be in the next release

mikeseese commented 6 years ago

This was released just now in ganache-core@2.1.3 and ganache-cli@6.1.4