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

Fix eclair-cli "compact" mode #2990

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix eclair-cli "compact" mode #2990

wants to merge 1 commit into from

Conversation

sstone
Copy link
Member

@sstone sstone commented Jan 29, 2025

"compact" mode (-s option) needs to be updated after #2969

"compact" mode (-s option) was broken after changes in how we manage short channel ids.
@sstone sstone requested a review from t-bast January 29, 2025 13:57
Copy link
Member

@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.

Good catch, that means I also need to update it in #2968

Copy link
Member

@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.

Actually can we wait and merge this after #2968 and make the changes for this PR as well at the same time? Otherwise I'll need to rebase...

@sstone
Copy link
Member Author

sstone commented Jan 29, 2025

Actually can we wait and merge this after #2968 and make the changes for this PR as well at the same time? Otherwise I'll need to rebase...

Yes it makes sense, I'll wait for #2968

@@ -155,7 +155,7 @@ jq_filter='if type=="object" and .error != null then .error else .';

# apply special jq filter if we are in "short" output mode -- only for specific commands such as 'channels'
if [ "$short" = true ]; then
jq_channel_filter="{ nodeId, shortChannelId: .data.shortIds.real.realScid, channelId, state, commitments: (.data.commitments.active | map({balanceSat: (try (.localCommit.spec.toLocal / 1000 | floor) catch null), capacitySat: .fundingTx.amountSatoshis, fundingTxIndex: .fundingTxIndex, channelPoint: .fundingTx.outPoint})) }";
jq_channel_filter="{ nodeId, shortChannelId: .data.shortIds.real, channelId, state, commitments: (.data.commitments.active | map({balanceSat: (try (.localCommit.spec.toLocal / 1000 | floor) catch null), capacitySat: .fundingTx.amountSatoshis, fundingTxIndex: .fundingTxIndex, channelPoint: .fundingTx.outPoint})) }";
Copy link
Member

Choose a reason for hiding this comment

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

I think there are other parts that don't work, .fundingTx doesn't exit? Shouldn't it be:

Suggested change
jq_channel_filter="{ nodeId, shortChannelId: .data.shortIds.real, channelId, state, commitments: (.data.commitments.active | map({balanceSat: (try (.localCommit.spec.toLocal / 1000 | floor) catch null), capacitySat: .fundingTx.amountSatoshis, fundingTxIndex: .fundingTxIndex, channelPoint: .fundingTx.outPoint})) }";
jq_channel_filter="{ nodeId, shortChannelId: .data.shortIds.real, channelId, state, commitments: (.data.commitments.active | map({balanceSat: (try (.localCommit.spec.toLocal / 1000 | floor) catch null), capacitySat: .capacity, fundingTxIndex: .fundingTxIndex, channelPoint: .commitInput.outPoint})) }";

Copy link
Member

Choose a reason for hiding this comment

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

And to make it work after #2968, we can extract the scid "directly" from the data, we'll need to look at the optional ChannelAnnouncement (not sure if mapping on an Option works in bash?).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there are other parts that don't work, .fundingTx doesn't exit? Shouldn't it be:

You mean after #2698 ? The fix in this PR works on master at 73ea751

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I was missing the custom serializer for Commitment in JsonSerializers that adds the fundingTx field.

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