-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add rich tuple decoder #1353
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 rich tuple decoder #1353
Conversation
ec59b89
to
df21e01
Compare
df21e01
to
7e24a3a
Compare
d3de612
to
1bdd964
Compare
I just found out that |
3a039a0
to
488a194
Compare
Awesome @banteg! I still need to dig into this PR a little more in depth, but on a high level, I'd like to see some more testing around the |
Thanks for the feedback. I agree this needs more testing. I initially went with dict because this format is accepted as input kwargs, so I will need to dig into it a bit more to see if namedtuples would work as args, but that change makes sense. One function's output can be another function's input, so I think it's important to keep the parsed versions compatible. |
Current progress with namedtuples: # s = (a=1, b=[2, 3, 4], c=[(x=5, y=6), (x=7, y=8), (x=9, y=10)])
# t = (x=11, y=12)
# a = 13
inputs = [
(1, [2, 3, 4], [(5, 6), (7, 8), (9, 10)]),
(11, 12),
13,
]
> result = [named_data_tree(*item) for item in zip(abi, inputs)]
> result
[(a=1, b=[2, 3, 4], c=[(x=5, y=6), (x=7, y=8), (x=9, y=10)]),
(x=11, y=12),
13]
> inputs == result
True Now I need to figure out some quirks to make it work with |
I've been playing with different ways to present an anon tuple and landed on This is how example from the first message now looks like: > market = dydx.getMarket(1)
(token='0x89d24A6b4CcB1B6fAA2625fE562bDD9a23260359', totalPar=(borrow=1284671939554912048936288, supply=1999868739240631893521211), index=(borrow=1014323761632160906, supply=1008162740160717992, lastUpdate=1558020975), priceOracle='0x787F552BDC17332c98aA360748884513e3cB401a', interestSetter='0xad91a0ddf799176a0A87a32Dafe8F3dd28479918', marginPremium=(value=0), spreadPremium=(value=0), isClosing=False)
> market.totalPar
(borrow=1284671939554912048936288, supply=1999868739240631893521211) These tuples now should work everywhere a tuple is expected. I've also added some tests, although not comprehensive. The thing I dislike is that I can't just copy this representation and paste into another terminal. Maybe it can be alleviated with some clever factory that converts kwargs into tuples, but we won't have anonymous tuple representation then.
|
04610a4
to
ade0669
Compare
NB: Tests fail due to issues unrelated to this PR. |
Sweet, thanks! I reran the tests, so hopefully those will fix themselves. I'll take a look at the code tomorrow! |
One idea to think about. If we define these two constructors, we can have copyable literal namedtuples which would require only one import to work. I think this feature is more important than concise anonymous representation. Also, should we call it
For example:
|
One problem with namedtuple I found: |
Unfortunately, since v5 is in beta now, we can't make any more breaking changes (until v6 which is a ways out). We'll need to keep
I like Tuple. I also wouldn't be opposed to something like
A few options that come to mind off the top of my head are:
It seems to me like the easiest approach might be to just revert to returning a regular tuple for a couple reasons: 1) it feels unexpected to me to return a different name than the one that was provided in the ABI, and 2) what happens if we strip an underscore name which then becomes the same as a name that didn't originally have an underscore? Although, I can see where it would also be unexpected to have a regular tuple returned if you pass in the decode flag, so I don't have a strong opinion there. I think we'll just have to be clear in the documentation. @pipermerriam I'd like to get your input here as well! |
I think it would be enough to add this check after the stripping: def foldable_namedtuple(fields):
fields = [field.lstrip('_') for field in fields]
+ if '' in fields or len(set(fields)) < len(fields):
+ return tuple
Example with fallback to tuple:
Example with
To me the latter example looks less predictable, so my vote is for reverting to a tuple on conflicting input. As for |
I've reverted the breaking change, now |
Another issue with namedtuples: I also noticed that events completely lack tuple support, I'll try to address that too. |
Vyper outputs scalars as tuple, so I addressed that to keep it compatible: # before
In [2]: contract.caller().totalSupply()
Out[2]: Tuple(out=347980466501289403984)
# after
In [3]: uni_eth.caller().totalSupply()
Out[3]: 347980466501289403984 |
e29cd72
to
e328c5b
Compare
What was wrong?
Rich objects can already be passed where tuples are expected as input, but there was no way to decode outputs as named objects even though their component names are usually available in the ABI.
Related to Issue #1267
How was it fixed?
There is a new
decode
option available when instantiating a contract which applies both to calls and contract caller. When enabled, all tuples/structs are decoded as namedtuples. They are kept compatible with the current API and should work everywhere a tuple is expected.This works via
web3._utils.abi.named_tree(abi, data)
which decodes function inputs/outputs provided their ABIs. Internally it works with dicts as tuples have multiple limitations on how their fields can be named.There are also two customized namedtuple factories:
foldable_namedtuple(fields)
which returns subclass of namedtuple which can be instantiated via a single argument, such thattype(x)(x) == x
. This is done to keep it compatible with_align_abi_input
.Tuple(**kwargs)
which works with literal representation, such thatTuple(x=1, y=2)
returnsTuple(x=1, y=2)
. This is useful when copying the output so it can be loaded back easily.This pull request also fixes tuple support for event parser.
Example
Cute Animal Picture