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

add inbound fees fields to channel event #164

Merged

Conversation

bufo24
Copy link
Contributor

@bufo24 bufo24 commented May 6, 2024

Adds needed fields to be compatible with the new fields added in this PR:

lightningnetwork/lnd#8723

Will change from draft to ready for review whenever the LND PR gets merged

@bufo24 bufo24 force-pushed the feature/add-inbound-fees-support branch 2 times, most recently from c58b42b to 9d6f4d0 Compare May 6, 2024 12:27
@alexbosworth
Copy link
Owner

I was thinking about calling this source_discount rather than inbound_fee to give more clarity towards what it is typically doing, reducing the outbound fee rate based on the source

@bufo24
Copy link
Contributor Author

bufo24 commented May 6, 2024

I know LND also calls it inbound fees, so it's nice to be consistent with the naming, so other devs directly understand that we're talking about the same thing, instead of having to figure out it source_discount and inbound fees are the same.

@alexbosworth
Copy link
Owner

I don't think it would be the same because I'd flip the sign on it, although I have to play with it to know

Consistency with naming isn't a goal of the arguments generally

@alexbosworth
Copy link
Owner

I'm playing around with this on the other APIs, just a heads up not 100% settled on naming but I can adjust it

@alexbosworth
Copy link
Owner

OK what I settled on is:

inbound_fee_base_msat should map to inbound_base_discount_mtokens but negative sign (unless 0, then 0), and a string rather than a number

inbound_fee_rate_milli_msat should map to inbound_rate_discount but negative sign (unless 0, then just 0)

@bufo24
Copy link
Contributor Author

bufo24 commented May 8, 2024

LND already gives the negative values, do you want to make them positive?

@bufo24
Copy link
Contributor Author

bufo24 commented May 8, 2024

And I tested this and I see that the inbound fees are coming in like numbers, not strings:

{
    custom_records: {
      '\x03Ù\x00\x00\x00\x00\x00\x00': <Buffer ff ff ff f6 ff ff ff ec>
    },
    time_lock_delta: 50,
    min_htlc: '1000',
    fee_base_msat: '10',
    fee_rate_milli_msat: '10000000',
    disabled: false,
    max_htlc_msat: '49500000',
    last_update: 0,
    inbound_fee_base_msat: -10,
    inbound_fee_rate_milli_msat: -20
  }

@alexbosworth
Copy link
Owner

Yeah I saw that too, I pushed some changes for the existing inbound fees that should show the transform

@bufo24 bufo24 marked this pull request as ready for review May 14, 2024 09:12
@bufo24 bufo24 force-pushed the feature/add-inbound-fees-support branch from ff81ea4 to d5ecb6c Compare May 14, 2024 09:15
@bufo24 bufo24 requested a review from alexbosworth May 30, 2024 09:10
@@ -117,16 +119,28 @@ module.exports = update => {

const txId = !!transactionId.equals(emptyTxId) ? null : transactionId;

const {
disabled,
Copy link
Owner

Choose a reason for hiding this comment

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

i don't think the other variables need to change

Copy link
Owner

@alexbosworth alexbosworth left a comment

Choose a reason for hiding this comment

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

can consolidate around the new format like in getchannel

@bufo24 bufo24 force-pushed the feature/add-inbound-fees-support branch from d5ecb6c to 01fce0f Compare May 30, 2024 16:01
@alexbosworth alexbosworth merged commit 51a5849 into alexbosworth:master May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants