validate geth_kwargs with a pydantic model

[pacrob]

What was wrong?

Currently geth_kwargs are passed around as a dict which does not allow type checking.

I considered using using TypedDict, but pydantic offers more flexibility in allowing the user to add additional values to be passed through to geth.

• Added pydantic as a dependency and added geth/utils/validation.py to house the new GethKwargs pydantic model- it's currently just a dump of all the geth commands I could find, will be cleaned up in a future PR.

• Adding types for geth/accounts.py

• Validate typing of geth_kwargs in accounts.py and reset.py and some directly connected functions.

• Changed return type of functions in geth/accounts.py from bytes to string. geth stdout and stderr are received as bytes, but otherwise it makes no sense to keep passing them around as bytes within py-geth, just a source of potential confusion. Updated tests that checked for byte-string versions of returned accounts.

• Bumped mypy to v1.10.0.

• Add "eval_type_backport>=0.1.0; python_version < '3.10'" to install_requires due to pydantic + future annotations issue: future annotations with Python 3.8 cause TypeError pydantic/pydantic#7873

How was it fixed?

Todo:

• Clean up commit history
• Add or update documentation related to these changes
• Add entry to the release notes

Cute Animal Picture

[kclowes]

changing **geth_kwargs in the signature to geth_kwargs changes the API from accepting any number of arguments to only accepting one. Was that an intentional choice? Curious what the tradeoffs you considered were. I think this change would have relatively far-reaching impact, so I'm not against it exactly, but want to make sure we're doing it with purpose and intention.

[pacrob]

Was that an intentional choice?

Indeed!

Curious what the tradeoffs you considered were.

My assumption/understanding is that anything being passed as **geth_kwargs is already collated as a dict by the user. was incorrect, found plenty of places where some kwargs are passed separately, then a dict using **. But it's still the same one additional step.

This does introduce the need to collate it as the pydantic model GethKwargs before passing it in, but that can be done with:

geth_kwargs_model = GethKwargs(key_a=True, key_b="cats", **geth_kwargs)

It introduces an extra step, but that step allows all the kwargs to be type-checked. The other option considered was to use a TypedDict, but where pydantic can be set to allow additional key-values that were not defined in the original model, typed dicts do not.

[kclowes]

Can we do something like:

def get_accounts(
    data_dir: str, **geth_kwargs: GethKwargs
) -> tuple[str, ...] | tuple[()]:
    """
    Returns all geth accounts as tuple of hex encoded strings

    >>> get_accounts()
    ... ('0x...', '0x...')
    """
    geth_kwargs = GethKwargs(**geth_kwargs)

    geth_kwargs.data_dir = data_dir
    geth_kwargs.suffix_args = ["account", "list"]
    
    ...

[pacrob]

Can we do something like:

def get_accounts(
    data_dir: str, **geth_kwargs: GethKwargs
) -> tuple[str, ...] | tuple[()]:
    """
    Returns all geth accounts as tuple of hex encoded strings

    >>> get_accounts()
    ... ('0x...', '0x...')
    """
    geth_kwargs = GethKwargs(**geth_kwargs)

    geth_kwargs.data_dir = data_dir
    geth_kwargs.suffix_args = ["account", "list"]
    
    ...

We could go that way. Internal calls to get_accounts would be converting it back and forth, which seems less than desirable.

Edit: thought about it more. I had approached this with the idea that eliminating the passing around of dicts was the most important aspect. But preserving signatures where possible is nicer for users. I'll reassess using the pydantic model just for data validation and otherwise leave as much as possible alone.

[fselmo]

lgtm 👍🏼

[fselmo]

I wonder actually if we should use a pattern like

from pydantic import create_model

# create a type for geth kwargs based off TypedDict so we can use `Unpack[GethKwargs]` in method signatures
class GethKwargs(TypedDict, total=False):
    arg1: str
    arg2: bool
    # etc...

# create the pydantic model for validation (and use this like we are currently using `GethKwargs` in this PR)
GethKwargsModel = create_model('GethKwargsModel', **GethKwargs)

... and then we can use Unpack[GethKwargs] instead of Any for typing them in method signatures. Any thoughts on that or alternate ways to not have to use Any?

[fselmo]

Kind of pseudocode above... but unpack the kwargs into a pydantic model separately.

kwargs_fields = {k: v for k, v in GethKwargs.__annotations__.items()}
GethKwargsModel = create_model('GethKwargsModel', **kwargs_fields)

---

Edited to reflect optional kwargs, not required (total=False and {k: v} instead of {k, (v, ...)}).

[pacrob]

Discussed elsewhere, but validating with a GethKwargs TypedDict causes argument overlap issues in functions such as get_accounts that have args that are also keys in geth_kwargs. Shelving for now.