Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

msggen: add missing-grpc command #7209

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

jackstar12
Copy link
Contributor

@jackstar12 jackstar12 commented Apr 11, 2024

This PR adds a missing-grpc command to msggen which can be used to track down missing grpc methods: #7167 (comment)
It currently outputs:

Missing RPC commands in grpc:
delforward
makesecret
reserveinputs
batching
upgradewallet
openchannel_signed
dev-forget-channel
renepaystatus
autoclean-status
delpay
fundchannel_complete
bkpr-listaccountevents
emergencyrecover
check
sql-template
splice_init
commando-blacklist
openchannel_abort
showrunes
getlog
listinvoicerequests
parsefeerate
multiwithdraw
openchannel_init
disableinvoicerequest
splice_signed
disableoffer
commando-listrunes
notifications
openchannel_bump
bkpr-dumpincomecsv
autoclean-once
checkrune
recoverchannel
fundchannel_start
deprecations
splice_update
bkpr-channelsapy
blacklistrune
sendonionmessage
recover
addpsbtoutput
unreserveinputs
listconfigs
plugin
createrune
fundchannel_cancel
commando-rune
listsqlschemas
commando
help
multifundchannel
renepay
openchannel_update
funderupdate
bkpr-inspect
bkpr-listbalances
sendinvoice
invoicerequest
setpsbtversion
setconfig

Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3b64d42

Building tooling around a temporary thing may appear weird, and I thought so too initially, but "temporary" had been the status quo for too long not to do this 😅

@ShahanaFarooqui
Copy link
Collaborator

@jackstar12 Could you please rebase the PR on origin/master and resolve the conflict?

@jackstar12 jackstar12 force-pushed the master branch 2 times, most recently from ecd47d2 to f252233 Compare June 26, 2024 21:04
@jackstar12
Copy link
Contributor Author

Just rebased:

Missing RPC commands in grpc:
deprecations
listsqlschemas
parsefeerate
commando-blacklist
notifications
batching
check
sql-template
commando-listrunes
commando-rune
commando

@ShahanaFarooqui ShahanaFarooqui added cln-grpc Highlight - UX and Dev Experience Fit and finish to make higher layer devs joyous and removed needs-rebase labels Jun 27, 2024
@ShahanaFarooqui
Copy link
Collaborator

ShahanaFarooqui commented Jun 27, 2024

@jackstar12 Thanks for the PR and adding the missing commands list code too :).

Also I would suggest that only six RPC commands are missing now:

1: deprecations
2: listsqlschemas
3: parsefeerate
4: notifications
5: batching
6: check

Below is the justification for others to be removed from the list:

  • sql-template: It is not an rpc command, just a template to generate final sql schema json with db details
  • commando-blacklist: Deprecated, will be removed v25.02. Encourage users to use blacklistrune instead.
  • commando-listrunes: Deprecated, will be removed v25.02. Encourage users to use showrunes instead.
  • commando-rune: Deprecated, will be removed v25.02. Encourage users to use createrune instead.
  • commando: Commando is difficult to define due to it's recursive nature (Reference).

@ShahanaFarooqui
Copy link
Collaborator

@jackstar12 Also please add a change-log entry including the list of rpc methods added with this PR.
Something like commit.

@cdecker
Copy link
Member

cdecker commented Jun 27, 2024

I would also.add that the two methods notifications and batching cannot be translated in their current form. Both act on the file descriptor, subscribing to notifications on the same client connection, and deferring DB commits on commands coming through the instrumented file descriptor.

Since there is no concept of a connection as such in grpc (it multiplexes commands over several connections so which connection you're instrumenting is not clear) and cln-rpc opens a new FD for every call as well, these methods have absolutely no effect and should not be mapped.

This leaves us with 4 methods to translate. Shall we maybe just mao them and skip adding a separate tool to identify unmapped methods?

@ShahanaFarooqui
Copy link
Collaborator

ShahanaFarooqui commented Jun 28, 2024

Keeping the logic to identify un-mapped methods might not be a bad idea. It will be helpful in identifying future new methods (eg. listaddresses) as well. We can also confirm it with added or changelog entries but this seems cleanest way to identify. Though we can filter-out ["sql-template", "commando-blacklist", "commando-listrunes", "commando-rune", "commando", "notifications", "batching"] methods to keep the list accurate.

Updated missing commands list:
- check
- deprecations
- listsqlschemas
- parsefeerate
- listaddresses (adding in v24.08) 

@rustyrussell
Copy link
Contributor

Note that some commands are either experimental or internal use only. We haven't documented this well in the past, so they get baked in: we should consider this in future.

@rustyrussell
Copy link
Contributor

We should also move this list of supported commands to the schema, where documentation can access it.

@rustyrussell rustyrussell merged commit feaac0e into ElementsProject:master Jul 5, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cln-grpc Highlight - UX and Dev Experience Fit and finish to make higher layer devs joyous
Projects
None yet
4 participants