search

mikeshultz / ledger-eth-lib

Library to interface with Ethereum app on Ledger hardware wallets
https://ledgereth.readthedocs.io/
MIT License
19 stars 9 forks source link

feat request: support HexBytes more #43

Closed antazoey closed 1 year ago

antazoey commented 1 year ago

so many places have assert type(thing) == bytes if we did assert isinstance(thing, bytes), it'd allow HexBytes

But also, assertion errors are not the best to encounter, the message is not helpful without digging into the tb; a TypeError("Expecting bytes, not blah blah") would be better for the lib

mikeshultz commented 1 year ago

I'm pretty hesitant to jump on HexBytes. Would kind of prefer to remain using primitives and not add a dependency. Keep this lib as simple as possible and let the consumer make decisions on things like that.

The assertion errors are all (I think) just to shut up mypy, not so much as input validation. I'm curious, where'd you run into it? It might be a bug.

antazoey commented 1 year ago

isinstance() will work for mypy too! And you dont have to explicitly support HexByes, it will just work. basically all mypy assertions that use type() instead of instance

mikeshultz commented 1 year ago

isinstance() will work for mypy too!

I get that, I only brought that up to say that you shouldn't be seeing these assertion errors. They aren't for users.

And you dont have to explicitly support HexByes, it will just work.

What does this mean?

antazoey commented 1 year ago

The assertions will be there unless you compile in optimized mode. And I mean I will be able to pass sub-classes instead of the asserted type and everything will work but you won't have to include the mentioning of HexBytes anywhere because itll still work but it wont fail the type check

mikeshultz commented 1 year ago

Oh, I think I misunderstood. isinstance(x, bytes) would allow HexBytes. You were not requesting adding isinstance(x, HexBytes) to allow derivatives of HexBytes. Yeah, I'm good with that.

Also, deja vu.