Skip to content

Specify that resolution of amount is msat#700

Merged
cdecker merged 3 commits into
lightning:masterfrom
Sword-Smith:patch-3
Jan 29, 2020
Merged

Specify that resolution of amount is msat#700
cdecker merged 3 commits into
lightning:masterfrom
Sword-Smith:patch-3

Conversation

@Sword-Smith
Copy link
Copy Markdown
Contributor

@Sword-Smith Sword-Smith commented Nov 14, 2019

When the p multiplier is used, the amount MUST be divisible
by 10 since the resolution used internally is millisatoshi.

This addresses but does not close- #692.

When the `p` multiplier is used, the amount MUST be divisible
by 10 since the resolution used internally is millisatoshi.

This addresses but does not close lightning#692.
@Sword-Smith
Copy link
Copy Markdown
Contributor Author

Sword-Smith commented Nov 17, 2019

I suggested the wording "MUST request amount divisible by ten if the p multiplier is used." but perhaps the wording "if the p multiplier is used, MUST request amount that is divisible by ten."?

Copy link
Copy Markdown
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

That sounds reasonable, ACK.

@t-bast t-bast added the Meeting Discussion Raise at next meeting label Dec 3, 2019
@cdecker
Copy link
Copy Markdown
Collaborator

cdecker commented Dec 9, 2019

ACK

@niftynei niftynei added the clarification substantive change or addition around wording or meaning label Jan 6, 2020
Comment thread 11-payment-encoding.md Outdated
@cdecker cdecker requested a review from t-bast January 21, 2020 13:01
@cdecker
Copy link
Copy Markdown
Collaborator

cdecker commented Jan 21, 2020

As discussed during the spec meeting on Monday 2020/01/20 I rephrased the requirement for trailing 0 decimal. I also added a short explanation to the rationale section, thus requiring new ACKs.

Ping @t-bast @Sword-Smith @Roasbeef @rustyrussell

Copy link
Copy Markdown
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK 29f1386

@Sword-Smith
Copy link
Copy Markdown
Contributor Author

I thought the wording was a bit off, made a fixup commit. Ping @cdecker @t-bast etc.

Copy link
Copy Markdown
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Woops good catch, ACK afba7a7

@cdecker
Copy link
Copy Markdown
Collaborator

cdecker commented Jan 29, 2020

Rebased and squashed

ACK 29f1386

@cdecker cdecker merged commit 17df7f2 into lightning:master Jan 29, 2020
Comment thread 11-payment-encoding.md
- if a specific minimum `amount` is required for successful payment:
- MUST include that `amount`.
- MUST encode `amount` as a positive decimal integer with no leading 0s.
- If the `p` multiplier is used the `amount` the last decimal MUST be `0`.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: @cdecker I think you missed a last comment that this sentence is weird.
It should be: "If the p multiplier is used the last decimal of the amount MUST be 0."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh damn, I thought there was a fixup from @Sword-Smith that fixed that. I'll open a new PR with the spelling mistake.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I did leave a fixup. But for some reason it didn't get included. I made a new PR
#733

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clarification substantive change or addition around wording or meaning Meeting Discussion Raise at next meeting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants