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

Amount representations: initial groundwork. #2353

Merged

Conversation

Projects
None yet
4 participants
@rustyrussell
Copy link
Contributor

commented Feb 14, 2019

This does some random cleanups and introduces the structures. The next series changes the APIs, and the final series changes us to use these types everywhere for better typesafety.

@rustyrussell rustyrussell requested a review from cdecker as a code owner Feb 14, 2019

@rustyrussell rustyrussell added this to the v0.7 milestone Feb 14, 2019

@wythe

This comment has been minimized.

Copy link
Collaborator

commented Feb 14, 2019

a hearty concept ACK to this!

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/amount-suffixes branch 2 times, most recently from 48cd48b to 5089535 Feb 15, 2019

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/amount-suffixes branch from 5089535 to c41b1f5 Feb 17, 2019

@rustyrussell

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2019

(Last force push just added brackets to constants inside macros...)

rustyrussell added some commits Feb 18, 2019

type_to_string: return const char *.
Always be const if you can.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
lightningd: re-enable IO logging for JSON output.
Hex format is terrible, but sometimes it's the only way to tell WTF is
going on.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@cdecker
Copy link
Member

left a comment

Nice changes , though I'm a bit in doubt whether having both struct amount_msat and struct amount_sat actually gives us more flexibility over a single struct amount that has some accessors that do the conversion to an actual u64 as late as possible.

I'd propose merging struct amount_sat and struct amount_msat into a single struct amount that is opaque in the header, and have amount_to_msat and amount_to_sat accessors that give you the u64 values converted to what you need. Otherwise we have loads of conversions to do simple operations, or loads of nearly identical functions (see 5e949f3). Ultimately it's up to the user code to decide whether they need msat, sat or btc.

@@ -935,6 +935,9 @@ void plugins_init(struct plugins *plugins, const char *dev_plugin_debug)
io_new_conn(p, stdin, plugin_stdin_conn_init, p);
req = jsonrpc_request_start(p, "getmanifest", p->log,
plugin_manifest_cb, p);
/* This might affect what APIs the plugin wants to expose */
json_add_bool(req->stream,
"allow-deprecated-apis", deprecated_apis);

This comment has been minimized.

Copy link
@cdecker

cdecker Feb 19, 2019

Member

What's the point of passing this in the getmanifest? Do you expect plugins to adhere to the deprecated apis as well and not expose some things depending on this flag? It'd be better to make this parameterless and just tell lightningd in the manifest whether something is to be considered deprecated or not. lightningd can then decide whether to expose them. If this is about telling the plugin which subset of RPC calls is available, the init is the right place for this.

This comment has been minimized.

Copy link
@cdecker

cdecker Feb 19, 2019

Member

Also since allow-deprecated-apis is not a valid variable name for most programming languages we should replace - with _ in the name here.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Feb 20, 2019

Author Contributor

OK, I'll do it that way (I was originally thinking we wouldn't expose deprecated functions at all).
Do we want args to getmanifest anyway, for future proofing? A plugin may change its interface based on other early args in future.

This comment has been minimized.

Copy link
@cdecker

cdecker Feb 20, 2019

Member

You're right, it might be nice to give the plugin some context even before completing the init-dance. Let's keep the args in there.

* rather than accessing the members directly. */
struct amount_sat {
/* Amount in millisatoshis. */
u64 satoshis;

This comment has been minimized.

Copy link
@cdecker

cdecker Feb 19, 2019

Member

Comment needs changing.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Feb 20, 2019

Author Contributor

Done...

@cdecker

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

Nevermind, after discussing this with @rustyrussell it seems to be a better solution having separate type for msatoshi and satoshi.

ACK c41b1f5

rustyrussell added some commits Feb 20, 2019

pylightning: prepare plugin for parameters to getmanifest.
We might still add these in future, so I think we should allow for it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
wire: move short_channel formatting functions into bitcoin/short_chan…
…nel_id

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
common/amount: new types struct amount_msat and struct amount_sat.
They're generally used pass-by-copy (unusual for C structs, but
convenient they're basically u64) and all possibly problematic
operations return WARN_UNUSED_RESULT bool to make you handle the
over/underflow cases.

The new #include in json.h means we bolt11.c sees the amount.h definition
of MSAT_PER_BTC, so delete its local version.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
update-mocks: make sure we can find json_add functions.
These are on start of line, which is unexpected.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
db: add amount functions.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
update-mocks: handle NO_NULL_ARGS and NON_NULL_ARGS functions.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/amount-suffixes branch from c41b1f5 to 487ad02 Feb 20, 2019

@rustyrussell

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

Rebased to fix the cut/paste bug in comment, and remove the deprecated flags. I left the ability to add params to getmanifest later, however (without that commit the python chokes on them).

@cdecker

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

ACK 487ad02

@niftynei
Copy link
Collaborator

left a comment

sorry super behind on PR's! mostly a bunch of questions.

struct command *cmd)
{
return command_done_err(cmd, JSONRPC2_METHOD_NOT_FOUND,
"%s has been deprecated", command->name);

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

This is nice. 💟

#define MSAT_PER_BTC (MSAT_PER_SAT * SAT_PER_BTC)

/* Use these to wrap amounts, for typesafety. Please use ops where possible,
* rather than accessing the members directly. */

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

Is there a way to make these 'package level visible' or is that a Java invention?

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

by "these" I mean the same 'members' that you mention in the comment.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Feb 20, 2019

Author Contributor

Yes, you can have it declared but not defined. Though it means:

  1. You can't embed it in structures, you need a pointer. Which means allocations all over...
  2. You can't pass it by copy.
  3. You can't access it externally, and our abstraction is a bit too leaky for that.

I thought about having the field name randomized at build time, but in practice the final commit simply adds to make check-source that nobody adds a new access to the internals :)

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 21, 2019

Collaborator

cool, just saw the check you added III 👍


/* You may not always be able to convert satoshis->millisatoshis. */
WARN_UNUSED_RESULT bool amount_sat_to_msat(struct amount_msat *msat,
struct amount_sat sat);

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

just curious: why is the second param not passed via pointer?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Feb 20, 2019

Author Contributor

It's usual to pass structs by pointer rather than copy, yes. But it's (really) just a u64, so copying is basically free, and passing by copy is easier to chain functions together which return a struct. esp AMOUNT_SAT(1000) works.


/* This may require rounding. */
WARN_UNUSED_RESULT bool amount_msat_to_sat_exact(struct amount_sat *,
const struct amount_msat *);

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

Unlike the method declaration above, these are missing method param 'names' i.e. struct amount_sat *sat

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Feb 20, 2019

Author Contributor

Hmm, that function is completely unused anyway: amount_msat_to_sat() does precisely the same thing. Will delete.

struct amount_sat amount_tx_fee(u32 fee_per_kw, size_t weight);

/* Different formatting by amounts: btc, sat and msat */
const char *fmt_amount_msat_btc(const tal_t *ctx,

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

Add an example comment for what the output looks like? e.g. 1.2btc ?

FAIL_MSAT(&msat, "0.00000000");
FAIL_MSAT(&msat, "0.00000000000");
FAIL_MSAT(&msat, "0.00000000msat");
FAIL_MSAT(&msat, "0.00000000000msat");

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

negative values?

PASS_MSAT(&msat, "0.00000000001btc", 1);
PASS_MSAT(&msat, "1.23456789btc", 123456789000);
PASS_MSAT(&msat, "1.23456789012btc", 123456789012);
FAIL_MSAT(&msat, "1btc");

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

why does this fail?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Feb 20, 2019

Author Contributor

We require decimal places for BTC. It seems unlikely anyone wants to actually send whole btc and not include 8 decimals (which is what bitcoind always does).

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 21, 2019

Collaborator

Ok cool! I saw all the notes you added to the docs about it 👍

{
struct amount_sat sat;

sat.satoshis = sqlite3_column_int64(stmt, col);

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

are these sqlite3_column_int64s unsigned?

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

i'm pretty sure these are signed ints https://www.sqlite.org/capi3ref.html#sqlite3_int64. storing them as 'blob64' uses the uint_64 type.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Feb 20, 2019

Author Contributor

Yes, but they're perfectly fungible, too. It'll just look a bit weird in the database. Perhaps we should do a sweep and change them all? But that's a separate cleanup; I'll put a FIXME.

Update: No, blob64 is something else (the 64 refers to the 64-bit blob length).

This comment has been minimized.

Copy link
@cdecker

cdecker Feb 20, 2019

Member

They are signed, which is why I used to have a check that we don't go over INT64_MAX but that seems to have been removed somewhere.

Anyway, storing u64 and then loading into u64 seems to be doing the right thing even when > INT64_MAX 😉

amount: minor comment and text improvements and remove unused function.
Reported-by: @niftynei
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@niftynei

This comment has been minimized.

Copy link
Collaborator

commented Feb 21, 2019

ACK 957c39b

@rustyrussell rustyrussell merged commit 023923e into ElementsProject:master Feb 21, 2019

2 checks passed

ackbot PR ack'd by niftynei
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.