Subtype Parsing#2511
Conversation
rustyrussell
left a comment
rustyrussell
left a comment
There was a problem hiding this comment.
Generally like it, not sure it technically needs to be based on the TLV branch though I can see it'd get messy to disentangle.
We should see if we can use it on various internal structures; all those places where we open-code fromwire_ and towire_. Eg. fromwire_route_info, fromwire_channel_config, fromwire_crypto_state, fromwire_basepoints, fromwire_added_htlc, etc. Some of them should be amenable I think?
| messages.append(Message(parts[0], Enumtype("WIRE_" + parts[0].upper(), parts[1]), comments)) | ||
| if parts[1] == '$': # this is a subtype | ||
| subtypes.append(Subtype(parts[0], comments)) | ||
| else: |
There was a problem hiding this comment.
$? Can we just have a single entry be a subtype?
Note that there's a companion to these changes, which is that the spec itself needs to decide on a format, and lightning-rfc/tools/extract-formats.py needs to be taught to extract it into the form you want here.
There was a problem hiding this comment.
updated the RFC parser to output desired format here: lightning/bolts#597
There was a problem hiding this comment.
OK, I moved my complaint to that PR :)
Otherwise, excellent. Be nice to try this out on some internal types, too: autogen FTW.
| funding_compose,32,num_inputs,2 | ||
| funding_compose,34,input_info,$num_inputs | ||
| funding_compose,34+$,num_outputs,2 | ||
| funding_compose,36+$,output_info,$num_outputs |
There was a problem hiding this comment.
Maybe insist that subtypes are defined before use, and then you can just recognize the typename here, no need for $?
There was a problem hiding this comment.
That makes the RFC's a bit harder to read, imo
There was a problem hiding this comment.
thinking about this more, we could swap out the $ with the type name in place, but to truly accurately represent the 'size' we need to know both the count field (if it exists) + the size of the subtype. using the $ was meant as a 'space holder for some amount of a subtype size calculation', but we could definitely replace it with num_outputs*input_info instead.
There was a problem hiding this comment.
this has been updated!
definitely! most of this piggy-backs on that infrastructure for building those already. i seem to remember one of them have some custom parsing logic, but otherwise that'd be a quick fix up. |
niftynei
force-pushed
the
dual-funded-subtype
branch
from
20a57f0 to
f40f398
Compare
niftynei
force-pushed
the
dual-funded-subtype
branch
4 times, most recently
from
cbd1aab to
718952c
Compare
make TLV messages their own subclass of Message. this makes other clean ups easier
first pass at adding subtype structs
this probably could be consolidated, as it splits out all the print_to/fromwire method stuff for the Subtype class
`m` might not be set on the optional set as well, so move this check down so that we now encompass both codepaths
This is needed so that some csv's can expose their subtype parsing functions in their header. This gets used in a later PR where we start replacing manually created 'subtype' definitions with generated ones.
the original version of the subtype generator emitted '$' to designate that a field was a subtype; now it's got a different format: funding_type,8,num_inputs,2 funding_type,10,input_info,num_inputs*input_info this patch updates our generator to understand this new format
subtype children should be allocated off of themselves. this was failing to compile for embedded subtypes (subtype within a subtype).
subtypes don't use the fance type registration that other 'set structs' do, see devtools/printwire.c
including tlv's in the wire docs breaks the printwire because there's a bad method name. this fixes that
now we print the subtypes out when you call printwire note that we have to reverse the order the subtypes appear in because 1) they're static and, 2) a few of them are nested
niftynei
force-pushed
the
dual-funded-subtype
branch
from
718952c to
4c27a05
Compare
|
Ack 4c27a05 |
2 participants
Adds parsing capabilities for 'subtypes' in csv structs.