New pyln modules.

[rustyrussell]

These will almost certainly see more work as I port the protocol tests across, but since I've never published a python module before, and have no idea what I'm doing, it's probably best to get some review!

Help!

[darosior]

Just quickly went through the code, this looks really nice !

Aside question : I once gave the protocol tests a shot (ElementsProject/lightning-rfc-protocol-test#13) which then seem discontinued, are we going to merge them here ?

[rustyrussell]

Just quickly went through the code, this looks really nice !

Aside question : I once gave the protocol tests a shot (ElementsProject/lightning-rfc-protocol-test#13) which then seem discontinued, are we going to merge them here ?

My final step will be to port the tests to this framework, then I will go through those PRs and merge. Could be a while yet though!

[rustyrussell]

Minor rebase:

1. Cleaned up commit msgs and added Changelog-Added lines.
2. Export SubtypeType and fix length check in case of missing fields.

[cdecker]

Wow, this PR is pretty meta :-) Apart from a few C-isms, some things that could be more pythonic and a couple of nits, it looks good to me.

• I'd prefer having a file-like interface on the serialization and deserialization primitives, since that allows us not to care about returning the remainder of the parsed string all over the place.
• I'm curious on how this continues in the next couple of PRs, but if it generates the type classes from the bolts I'd suggest adding them to the __all__ field which describes the exports. That'd just make the types importable by name. There's a bit of a roundabout way to do it, but in user ergonomics it works quite nicely:

import sys

class Message(object):
    def __init__(self, messagetype, **kwargs):
        self.messagetype = messagetype
        self.fields = {}

## Get the current module so we can append newly generated classes to it
mod = sys.modules[Message.__module__]

for name in ['A', 'B', 'C']:
    setattr(mod, name, Message(name))

This will create an instance of Message and register it as an importable object with the correct name. From the user of the module you can then just do the following:

from test import A, B, C

print(A)

[rustyrussell]

Wow, this PR is pretty meta :-) Apart from a few C-isms, some things that could be more pythonic and a couple of nits, it looks good to me.

• I'd prefer having a file-like interface on the serialization and deserialization primitives, since that allows us not to care about returning the remainder of the parsed string all over the place.

That works well for the binary ones (done), but it's less of a win for the string ops: it's more convenient to have them return a str. They're mainly used for debugging. Using a io.TextIOBase() for string input would be mainly an internal change, but probably even less efficient given how we split fields.

• I'm curious on how this continues in the next couple of PRs, but if it generates the type classes from the bolts I'd suggest adding them to the __all__ field which describes the exports. That'd just make the types importable by name. There's a bit of a roundabout way to do it, but in user ergonomics it works quite nicely:

Hmm, that's actually a nice trick. It's a bit tricky because message types exist in a "namespace" for good reasons (users, like protocol tests, can add/remove/modify the message types).

So it actually makes sense then to have boltX as individual modules, containing all the Message() types by name.

The problem is, for lnprototest, we actually replace the 'signature' type: since bolts are simply a csv which gets interpreted, we do the following:

ns = MessageNamespace()
# Replace fundamental type
ns.subtypes['signature'] = Sig
ns.add_csv(bolt1_csv + bolt2_csv + bolt7_csv)

I guess we could override messages.signature before importing... let me try.

[cdecker]

Forgot to mention this yesterday, but this is rather impressive code 👍

That works well for the binary ones (done), but it's less of a win for the string ops: it's more convenient to have them return a str. They're mainly used for debugging. Using a io.TextIOBase() for string input would be mainly an internal change, but probably even less efficient given how we split fields.

Good point, I was mainly thinking about the binary representations. The string and hex representations can then be wrappers around the binary ones, e.g., hex-decoding a given string and wrapping it in a BytesIO instance before passing it to the binary deserializer.

The problem is, for lnprototest, we actually replace the 'signature' type: since bolts are simply a csv which gets interpreted, we do the following:

ns = MessageNamespace()
# Replace fundamental type
ns.subtypes['signature'] = Sig
ns.add_csv(bolt1_csv + bolt2_csv + bolt7_csv)

I guess we could override messages.signature before importing... let me try.

Getting deep into the monkey-patching weeds here, but it is possible to replace after importing the module quite easily as well, as long as we get all the aliasing right (import the dependency, and then monkey patch its imported fields, hoping they didn't stash away an alias to the original and are using it somewhere).

[cdecker]

The cleanups are very nice, thanks for the fixups. I just noted some minor stylistic things, that can also be addressed at a later point in time.

One thing that threw me off in the first review round is that I usually use
the class as the type, doing things like the following to create a new
instance from a serialized string:

class Pubkey:
    def __init__(self, raw: bytes) -> None:
        self.raw: bytes = raw

    @classmethod
    def from_hex(cls: Type[T], h: Union[str, bytes]) -> T:
        if isinstance(h, str):
            h = h.encode('ASCII')

        return cls(
            raw=binascii.unhexlify(h),
        )

    def __str__(self) -> str:
        return binascii.hexlify(self.raw).decode('ASCII')

    def __repr__(self) -> str:
        return self.__str__()

Notice the from_hex method which is a class method, i.e., it receives the
class and not an instance as its first argument. This allows writing generic
methods that return the desired type, without having to copy-paste the
implementation every time.

In your code there is an explicit distinction between type, expressed as a
distinct class, and the instance class. This is fine, I was just totally
confused by it :-)

[cdecker]

What's the advantage over the following?

try:
    struct.unpack_from(structfmt, io)
except struct.error:
    if empty_ok:
        return None
    else:
        raise

Presumably you want to avoid raising the exception at all when it can be avoided?

[cdecker]

This could be weakened a bit to be

assert(self.get_type(t.name) is None or self.get_type(t.name) == t)

Which would no longer fail outright if we happen to add the same type multiple times.

[cdecker]

An idiomatic way of doing this is with the getter and default value:

return self.fundamentaltypes.get(name, None)

[cdecker]

These two if-statements can be joined.

[cdecker]

Seems you were far quicker than me finding out about recursive type definitions being possible via string annotations 😉

I got quite lost when trying to figure out how to annotate something with a type that'd be defined later...

[cdecker]

Are these only required for unit tests? In that case we could also not expose them here, and have the unit tests reach directly into .array_types

from pyln.proto.message.array_types import SizedArrayType, DynamicArrayType, EllipsisArrayType

If that helps keep the interface clean for the actual protocol tests.

[cdecker]

This can also be achieved with this:

for key, val in kwargs.items():
    self.set_field(key, val)

[cdecker]

After going through the comments once more I think we're actually good with this PR.

ACK 5dbec8f