Skip to content

Currency conversion support for bookkeeper if bkpr-currency is provided, and bkpr-report generic command#8937

Open
rustyrussell wants to merge 27 commits intoElementsProject:masterfrom
rustyrussell:guilt/bkpr-currency
Open

Currency conversion support for bookkeeper if bkpr-currency is provided, and bkpr-report generic command#8937
rustyrussell wants to merge 27 commits intoElementsProject:masterfrom
rustyrussell:guilt/bkpr-currency

Conversation

@rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Mar 11, 2026 •
edited
Loading

This involved a change to the currencyrate plugin's API, now I'm using it for more than just offers: I hope @daywalker90 doesn't mind. I got some help from ChatGPT here, but it made sense.

There is also a generic report-producing API which can take advantage of this

This Fixes #8915

@rustyrussell rustyrussell added this to the v26.04 milestone Mar 11, 2026
daywalker90
daywalker90 requested changes Mar 11, 2026
View reviewed changes
@daywalker90
Copy link
Collaborator

daywalker90 commented Mar 11, 2026

I have reviewed the currencyrate plugin changes and they are fine, except that one review above. The schemas also look correct to me. Should these commands be added to msggen?

@rustyrussell rustyrussell force-pushed the guilt/bkpr-currency branch 2 times, most recently from f312502 to d354ab8 Compare March 12, 2026 00:14
daywalker90
daywalker90 requested changes Mar 12, 2026
View reviewed changes
@daywalker90
Copy link
Collaborator

daywalker90 commented Mar 12, 2026

I reviewed the changes to the existing tests for currencyrate aswell. I have not reviewed the new tests with the bookkeeper integration.

@rustyrussell rustyrussell force-pushed the guilt/bkpr-currency branch 3 times, most recently from 544ef63 to d39273d Compare March 13, 2026 06:09
@rustyrussell
Copy link
Contributor Author

rustyrussell commented Mar 13, 2026 •
edited
Loading

I have reviewed the currencyrate plugin changes and they are fine, except that one review above. The schemas also look correct to me. Should these commands be added to msggen?

I think that's automagic now?

Ah there's a utils.py list :( But when I added it it complained about:

  File "/home/rusty/devel/cvs/lightning/contrib/msggen/msggen/patch.py", line 75, in visit
    assert added or f.added, f"Field {f.path} does not have an `added` annotation"
           ^^^^^^^^^^^^^^^^
AssertionError: Field ListCurrencyRates does not have an `added` annotation
make: *** [Makefile:429: cln-grpc/proto/node.proto] Error 1

Which it does have, so I'm confused.

@rustyrussell rustyrussell marked this pull request as ready for review March 13, 2026 06:28
@rustyrussell rustyrussell requested a review from cdecker as a code owner March 13, 2026 06:28
@daywalker90
Copy link
Collaborator

daywalker90 commented Mar 13, 2026

I have reviewed the currencyrate plugin changes and they are fine, except that one review above. The schemas also look correct to me. Should these commands be added to msggen?

I think that's automagic now?

Ah there's a utils.py list :( But when I added it it complained about:

  File "/home/rusty/devel/cvs/lightning/contrib/msggen/msggen/patch.py", line 75, in visit
    assert added or f.added, f"Field {f.path} does not have an `added` annotation"
           ^^^^^^^^^^^^^^^^
AssertionError: Field ListCurrencyRates does not have an `added` annotation
make: *** [Makefile:429: cln-grpc/proto/node.proto] Error 1

Which it does have, so I'm confused.

It is, wich is why i made #8873 . I always cheated when i added new commands by forcing all new added annotations in patch.py. After 8873 that is no longer necessary, but everything in new commands needs the added annotations then. So the first thing you'll get after 8873 and adding "ListCurrencyRates" will be AssertionError: Field ListCurrencyRates.currency does not have an added annotation because the currency doesn't have an added annotation. We could also make more changes to msggen to assume it's the same version as when the command was added, but we'd have to check if it's the first time adding the command etc.. If you don't want to merge 8873 right now you can do the patch.py trick like this:

Uncomment and change to this:

 if f.added is None and 'added' not in m:
       m['added'] = 'v26.04'

run uv make gen

git restore contrib/msggen/msggen/patch.py

@rustyrussell rustyrussell force-pushed the guilt/bkpr-currency branch 4 times, most recently from 609a96c to de29e73 Compare March 15, 2026 06:47
@rustyrussell rustyrussell changed the title Currency conversion support for bookkeeper if bookkeeper-currency is provided. Currency conversion support for bookkeeper if bkpr-currency is provided, and bkpr-report generic command Mar 16, 2026
@rustyrussell rustyrussell requested a review from niftynei March 16, 2026 02:43
@rustyrussell
pytest: test for currencyrate median code when values are cached.
943c93f 
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
currencyrate: fix cached currencies to still use median.
1dfa517 
It was using *only* the latest one, not all the fresh ones.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
currencyrate: don't sleep for as long in background task.
f0eea5c 
We process at least one stale entry per iteration (more if it has
changed by +/- 1%), so we should sleep for less if we have more
entries, in order to average hitting each source once per hour.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
currencyrate: make oracle store and serve the BTC price, not msat per…
f037b85 
… fiat unit.

This is the first step in updating the API.

Subtly, the median of BTC prices is different from the median of msat-per-dollar,
given it's the invoice.  This changes our test:

```
FAILED tests/test_currencyrate.py::test_cached_median - assert 133333 == 150000
```

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
currencyrate: expose get_median_rate and do conversion in front-end f…
ebffc84 
…or currencyconvert.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
currencyrate: change currencyrates to listcurrencyrates which is …
33f49e4 
…more CLN-ish.

And add a schema file.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `listcurrencyrates` API for examining the current values from the `currencyconvert` plugin's sources.
@rustyrussell
currencyrate: implement currencyrate command.
b660cda 
For the simple case of offers conversion, converting a given amount of
fiat to BTC is the desired API.  But for almost everything else, we
want to get the (median) per-BTC rate.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `currencyrate` API for getting the current median BTC conversion top a given fiat currency.
@rustyrussell
doc: add missing schema for currencyconvert API.
3356f5d 
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
pytest: clean up test_currencyrate.py using wait_for_logs()
f0f48b6 
It's explicitly designed for "these will occur, don't know what order".

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
bkpr: add bkpr-currency option to record currency rates on each event.
10bd884 
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: Config: `bkpr-currency` option to record conversion rate at each event.
@rustyrussell
bookeeper: add currencyrate to bkpr-listaccounts if we recorded it.
720ab6d 
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
bookkeeper: keep up with new entries if we're doing currency conversion.
a1e7685 
If we're doing currency conversion, we can't refresh lazily on each
command: we need to watch for new events and refresh then.  This is
fairly easy to do, however.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
bookkeeper: avoid wasteful refresh races.
9b7fa74 
Now we're refreshing more regularly, so it's worth avoiding races
rather than simply letting them happen and ignoring the results.

Keep a pointer in bkpr and piggy back on that one if it's there.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
libplugin: hand command, not plugin, to parsing functions.
bac87f0 
In particular, for dynamic options we want to create an aux_command in
the case of bookkeeper-currency.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
bookkeeper: make bookkeeper-currency dynamic.
089728f 
This means doing more work if we set it; in particular, we need to
clean up any old values.  We define the empty string as "unset", since
setconfig has no other way to "unset" variables.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
patch fix.patch
2154083
@rustyrussell
common: add mkdatastorekey helper to make string arrays.
f0fa2af 
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
common: add simple unit test for mkdatastorekey.
aff16bc 
Thanks ChatGPT!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
plugins: simplify datastore interfaces now callers have mkdatastorekey.
f05549f 
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
bookkeeper: persist currency conversions in the datastore.
055bb44 
For now we simply save every one.  Later we will get more efficient when they are
duplicates (as often happens).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
bkpr: warn if currency rate request is for old event.
516cb2b 
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@daywalker90 daywalker90 mentioned this pull request Mar 19, 2026
Merged
@rustyrussell
bookkeeper: save currencyrate ranges, not individual values.
e04b489 
The currencyrate plugin aims for 10 minute polls, so it just doesn't
move that fast.  If we're busy, recording the rate every second is
overkill.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
bookkeeper: expose currencyrate_str and bkpr_of so report.c can acces…
8b95e4d 
…s them.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
bookkeeper: use iso4217 check, scale and print correct minor units.
c209624 
e.g. USD = 2, CLF = 4, XAU = 0.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
cli: remove \ in simple format.
41cb9ae 
This does mostly the right thing, but this removes e.g. \" -> ", and \\ to \.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
bookkeeper: implement flexible "bkpr-report" command.
aa1b910 
You can specify the format for each line in a flexible way, using
format tags (inclding defaults if they are not set, recursively).

Changelog-Added: JSON-RPC: `bkpr-report` allows flexible summaries of bookkeeper income.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
msggen: add currencyconvert, currencyrate and listcurrencyrates.
5de8b0f 
I used daywalker's hack to patch.py to make this work:

```
 if f.added is None and 'added' not in m:
       m['added'] = 'v26.04'
```

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daywalker90 daywalker90 Awaiting requested review from daywalker90

@cdecker cdecker Awaiting requested review from cdecker cdecker is a code owner

@niftynei niftynei Awaiting requested review from niftynei

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v26.04

Development

Successfully merging this pull request may close these issues.

bookkeeper needs to record the currency conversion rate at the time of each event

2 participants

@rustyrussell @daywalker90