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

lnwire: Adjust Stringer to standard chan id format #2432

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

Conversation

alexbosworth
Copy link
Contributor

This would adjust logging output to use a more easily selectable short channel id components string

This would adjust logging output to use a more easily selectable short channel id components string
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@cfromknecht
Copy link
Contributor

Do we want to wait on merging until lncli can handle parsing of this format as well? For example, users who grab sids from logs and look them up via will need to manually replace the x’s with colons when querying

@cfromknecht cfromknecht closed this Jan 9, 2019
@cfromknecht
Copy link
Contributor

Oops!

@cfromknecht cfromknecht reopened this Jan 9, 2019
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM! 🎸

@Roasbeef
Copy link
Member

Would say we should wait on this until we update lncli to accept this format and convert it to the regular format. We can even just keep our RPCs as is for time being, and expect that lncli or the client is able to convert the short channel ID properly.

@Roasbeef Roasbeef added logging Related to the logging / debug output functionality spec P3 might get fixed, nice to have labels Jan 16, 2019
@guggero
Copy link
Collaborator

guggero commented Nov 21, 2020

What's the dependency for this? What needs to be changed in lncli to get this in?

@Roasbeef
Copy link
Member

What's the dependency for this? What needs to be changed in lncli to get this in?

I don't think anything does? Given that lncli always accepts the integer version since it's generally easier to copy/paste. We could also log the integer version in parens or something in this new stringer.

@Roasbeef Roasbeef added this to Reviewer approved in v0.13.0-beta Jan 28, 2021
@cfromknecht
Copy link
Contributor

What's the dependency for this? What needs to be changed in lncli to get this in?

Ideally all of our commands in lncli where a channelid is accepted we also accept the new format. Otherwise users will need to convert back into the integer format, e.g. when trying to use getchaninfo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logging Related to the logging / debug output functionality P3 might get fixed, nice to have spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants