cln-rpc & cln-grpc: Backfill more methods #5104
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdecker
force-pushed
the
cln-grpc-methods
branch
3 times, most recently
from
93c10c2
to
f0a5b05
Compare
|
Yeah, we really need to add these request schemas to to python framework for testing! OK, this isn't complete, but my proposal can be found (on top of this!) here: https://github.com/rustyrussell/lightning/tree/guilt/pr-5104 |
|
OK, my tree now has all the schema fixes, and some others. If you don't want to mess up your parsing, you can just cherry pick some of those changes (and not the part which actually validates it against the testsuite). |
cdecker
force-pushed
the
cln-grpc-methods
branch
from
876ada5
to
ed6e844
Compare
|
I incorporated @rustyrussell's changes and fixes and got things working again. Currently backfilling some more types since they can be hard and might be considered braking changes if we do it later. Also added a couple of tests to gain some more certainty that the conversion code is working. Will finish the changes by Monday, hoping to get these in asap |
cdecker
force-pushed
the
cln-grpc-methods
branch
from
6cbe4fe
to
9d34284
Compare
cdecker
force-pushed
the
cln-grpc-methods
branch
4 times, most recently
from
d70c2ce
to
c88effb
Compare
cdecker
force-pushed
the
cln-grpc-methods
branch
from
c88effb
to
0579969
Compare
|
Some commands have changed schemas maybe? Needs rebasing I think... |
|
Didn't get time to fold these, but: diff --git a/doc/schemas/signmessage.request.json b/doc/schemas/signmessage.request.json
index f5fd75727..163239e20 100644
--- a/doc/schemas/signmessage.request.json
+++ b/doc/schemas/signmessage.request.json
@@ -2,7 +2,7 @@
"$schema": "http://json-schema.org/draft-07/schema#",
"type": "object",
"required": [
- "id"
+ "message"
],
"additionalProperties": false,
"properties": {
diff --git a/tests/test_pay.py b/tests/test_pay.py
index bf8adf148..f4f5cc33b 100644
--- a/tests/test_pay.py
+++ b/tests/test_pay.py
@@ -2136,12 +2136,12 @@ def test_setchannel_routing(node_factory, bitcoind):
l1.rpc.getroute(l3.info['id'], 4001793, 1, fuzzpercent=0)["route"]
# We should consider this unroutable! (MPP is disabled!)
- inv = l3.rpc.call('invoice', {'msatoshi': 4001793,
- 'label': 'test_setchannel_1',
- 'description': 'desc',
- 'dev-routes': []})
+ inv = l3.dev_invoice(msatoshi=4001793,
+ label='test_setchannel_1',
+ description='desc',
+ dev_routes=[])
with pytest.raises(RpcError) as routefail:
- l1.rpc.dev_pay(inv['bolt11'], use_shadow=False)
+ l1.dev_pay(inv['bolt11'], use_shadow=False)
assert routefail.value.error['attempts'][0]['failreason'] == 'No path found'
# 1337 + 4000000 * 137 / 1000000 = 1885 |
|
Folded those simple fixes... |
rustyrussell
force-pushed
the
cln-grpc-methods
branch
from
0579969
to
94cf5db
Compare
|
Hmm, I put some cruft test changes in the rebase somehow. Fixed and once it's tested locally I'll push again... |
rustyrussell
force-pushed
the
cln-grpc-methods
branch
from
b655d28
to
6c29b48
Compare
Otherwise we get less nicely consecutive numbers because request and response share one domain. This separates them again.
This will override the schema later. Signed-off-by: Rusty Russell <[email protected]>
This will override the schema later. Signed-off-by: Rusty Russell <[email protected]>
lightningd is happy, but our schema won't be. Signed-off-by: Rusty Russell <[email protected]>
These are useful for requests: 1. "outpoint": <txid>:<outnum> 2. "feerate": strings or a number 3. "outputdesc": bitcoin-style addresses-as-keys 4. "msat_or_all": amount or "all" 4. "msat_or_any": amount or "any" 5. "short_channel_id_dir": scid with /0 or /1. Signed-off-by: Rusty Russell <[email protected]>
Types are fixed, in particular: * rename "OutputDesc" to more consistent "outputdesc". * rename "utxo" to more consistent "outpoint". * it's "boolean" not "bool". * "number" means int or float, usually it should be u32. Specific commands: * close `id` can be by channel id, scid. * close `feerange` is a feerate type. * datastore/deldatastore/listdatastore `key` can be singleton. * delexpiredinvoice: `maxexpirytime` is not required, is a u64. * invoice/delinvoice/listinvoice `label` can be an integer * fundpsbt: many fields are u32 not number (JSON for int or float). * invoice: `msatoshi` can be "any". * invoice: `expiry` has a type (now must be numeric). * invoice: `exposeprivatechannels` can be bool or array of scids. * invoice: `deschashonly` added * keysend: there's no "float" type, use "number" or "u32" etc. * keysend: `routehints` is a valid arg, as is `extratlvs` (EXPERIMENTAL_FEATURES) * listdatastore: `key` is not required. * newaddr: `addresstype` can be "all" * pay: `exemptfee` is "msat", new fields `locaofferid` and `exclude` * sendonion: was mis-formatted, missed `localofferid` and `groupid` fields. * sendpay: add `localofferid` and `groupid` params. * signpsbt: add `signonly` param. * txprepare "outptus" typo. * waitsendpay: add `groupid` and fix `partid` type. * withdraw: `destination` is a bitcoin address, not a pubkey. Signed-off-by: Rusty Russell <[email protected]>
This means suppressing schemas in some places too. Signed-off-by: Rusty Russell <[email protected]>
Sometimes we just want to paper over the schema directly. Mostly useful to sidestep the `oneof` things that are required for expressiveness.
These json structs are gigantic, so let's externalize them a bit.
Got some issues parsing dates for example.
It's schema definition is weirdly asymmetric, with variants dependent on another fields' value. Need to decide if we want to either hand-code a superset or make a more complex decoding, but definitely not something we'd want the generator to be able to do.
So far we were papering over the actual error with a generic string that isn't very useful. Now we decode the error and forward it through the grpc layer as well (though still as a string).
- disconnect - feerates - getroute - listforwards - listpays - ping - signmessage
cdecker
force-pushed
the
cln-grpc-methods
branch
from
e6db99f
to
fc6c054
Compare
|
Rebased, and addressed flake. |
cdecker
force-pushed
the
cln-grpc-methods
branch
from
fc6c054
to
0b0c06c
Compare
Signed-off-by: Rusty Russell <[email protected]>
Didn't want to pollute each commit with the changes in the derived files so here are all the changes in one commit :-)
cdecker
force-pushed
the
cln-grpc-methods
branch
from
0b0c06c
to
9a76d84
Compare
|
Ack 9a76d84 Remaining memleak flake is known, on my workaround list soon. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I now know how @rustyrussell felt filling in the JSON-Schemas for the JSON-RPC responses. This PR mostly consists of me backfilling the JSON-RPC request schemas, and then autogenerating the derived files, occasionally addressing a conversion issue or a missing type.
The PR is based on topp of #5013 and starts with the
msggen: Add meta file to keep the field numbers stablecommit (daa26ff at the time of writing).The following files where changed manually, all others were generated and are unlikely to hold any surprises:
cln-grpc/proto/primitives.prototo add theFeerate,UtxoandOutputDescprimitivescln-grpc/src/pb.rsto add a number of protobuf <-> JSON-RPC conversionscln-rpc/Cargo.tomlto add thehexdependencycln-rpc/src/primitives.rscontrib/msggen/msggen/__main__.pyto allowlist more RPC methods and add method renaming forconnectwhich is already used by grpc internallycontrib/msggen/msggen/grpc.pyto add the fixed mapping from field path to field number required for grpc to work across versions (renumbering fields is not allowed by protobuf and grpc since that's how fields are identified).contrib/msggen/msggen/model.pyandcontrib/msggen/msggen/rust.pyto add mapped typesdoc/schemas/connect.request.jsondoc/schemas/createinvoice.request.jsondoc/schemas/createonion.request.jsondoc/schemas/datastore.request.jsondoc/schemas/deldatastore.request.jsondoc/schemas/delexpiredinvoice.request.jsondoc/schemas/delinvoice.request.jsondoc/schemas/fundpsbt.request.jsondoc/schemas/invoice.request.jsondoc/schemas/keysend.request.jsondoc/schemas/listdatastore.request.jsondoc/schemas/listinvoices.request.jsondoc/schemas/listnodes.request.jsondoc/schemas/listsendpays.request.jsondoc/schemas/listtransactions.request.jsondoc/schemas/newaddr.request.jsondoc/schemas/pay.request.jsondoc/schemas/sendonion.request.jsondoc/schemas/sendpay.request.jsondoc/schemas/sendpsbt.request.jsondoc/schemas/signpsbt.request.jsondoc/schemas/utxopsbt.request.jsondoc/schemas/waitanyinvoice.request.jsondoc/schemas/waitinvoice.request.jsondoc/schemas/waitsendpay.request.jsondoc/schemas/withdraw.request.jsonIt is likely easiest to review the final diff and not the individual commits, since I jumped around a bit between methods that are more or less related. As for completeness you can look at
contrib/msggen/msggen/__main__.pyas to which methods are mapped and which aren't yet. Please let me know if there are any methods that are needed for your use-cases and that should be mapped.This has to be considered experimental code, since I couldn't test most of these commands, due to the sheer surface size. The code compiles, but we make liberal use of panics, which may lead to unexpected results. You have been warned, but feedback is very much welcome to stabilize this interface.