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

Add parity-specific listStorageKeys RPC. #1145

Merged
merged 3 commits into from
Nov 27, 2018
Merged

Conversation

palkeo
Copy link

@palkeo palkeo commented Nov 22, 2018

What was wrong?

There was no support for the parity_listStorageKeys RPC:
https://wiki.parity.io/JSONRPC-parity-module#parity_liststoragekeys

How was it fixed?

I added a function to implement it in the parity module.

How was it tested?

I tested locally, and also ran the parity integration tests.
I added a test, but because the current integration tests doesn't seem to use "--fat-db", parity returns None. I make sure it does.

However, the test is run in two variants, and one fails:
tests/integration/parity/test_parity_http.py::TestParityTraceModuleTest::test_list_storage_keys_no_support[<lambda>]

Where this one passes:
tests/integration/parity/test_parity_http.py::TestParityTraceModuleTest::test_list_storage_keys_no_support[address_conversion_func1]

The failure is because the address seems to be passed as raw bytes, and cannot be serialized to json. That's supposed to be allowed? How to handle that? I don't see code to handle it in the other RPCs.

Cute Animal Picture

   \
    \ /\
    ( )
  .( o ).

(that's an ASCII Bunny viewed from behind)

@carver
Copy link
Collaborator

carver commented Nov 23, 2018

Thanks!

The failure is because the address seems to be passed as raw bytes, and cannot be serialized to json. That's supposed to be allowed?

Yes, we allow three formats:

  • EIP-55 checksum address (0x + 40 hex mixed-case characters)
  • 20 bytes
  • ENS name

How to handle that? I don't see code to handle it in the other RPCs.

We have a middleware that looks for address inputs, which we have defined here:

RPC_ABIS = {
# eth
'eth_call': TRANSACTION_PARAMS_ABIS,
'eth_estimateGas': TRANSACTION_PARAMS_ABIS,
'eth_getBalance': ['address', None],

See eth_getBalance for example.


Also, note that another PR should be opened as a backport to the v4 branch, unless you're okay waiting a while before this feature is out in a stable web3 release. (I'd recommend waiting until this PR has merged before doing the backport, to minimize duplicate work)

@palkeo
Copy link
Author

palkeo commented Nov 27, 2018

I registered my function in the middleware you pointed me to. It seems to work now :)
And I'm totally fine waiting until it's in the stable release.

@@ -57,6 +57,8 @@
'personal_unlockAccount': ['address', None, None],
'personal_sign': [None, 'address', None],
'trace_call': TRACE_PARAMS_ABIS,
# parity
'parity_listStorageKeys': ['address', None, None, None],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine for now, but in general, we probably want a different solution. It seems weird to have this included in this default middleware, for a non-default module. (Comment mostly intended for maintainers)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking about

  • global middlewares (our current solution)
  • module middlewares (what we'd need to keep this change out of the global namespace and only on the web3.parity module)
  • method normalizers (part of what we talked about in the asyncio work of moving the pythonic middleware functionality into the methods themselves).
  • contract middleware (middlewares that are only used for web3 interactions initiated from a Contract).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, that's familiar. I guess in that framework this is a method normalizer, really, since it depends on argument type and order.

Contract middleware is one I don't remember talking about. Do you have an example? Are you thinking of attaching a middleware to a specific contract instance, or across all, or what?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attaching a middleware to a specific contract instance

This, though thinking through it there isn't a lot that this gives us that couldn't be accomplished by pushing users towards using separate w3 instances in cases where they want different behavior across different contract instances (think in-flight signing, or gas estimations).

@carver
Copy link
Collaborator

carver commented Nov 27, 2018

I love the ascii bunny, too XD

@carver carver merged commit e4cba0e into ethereum:master Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants