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.

Sign up for GitHub

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

Jump to bottom

lnrpc: set JS_STRING option for chan_id #2352

Merged
merged 1 commit into from
Oct 16, 2019
Merged

lnrpc: set JS_STRING option for chan_id #2352

merged 1 commit into from
Oct 16, 2019

Conversation

sangaman
Copy link
Contributor

This sets the jstype option to JS_STRING for all chan_id fields in the proto rpc definition. chan_id is a 64 bit integer, which is not natively supported by javascript's floating-point number with only 52 bit precision. Nevertheless, by default protobuf will use the number type for 64 bit integer fields in javascript, which can cause loss of precision problems with chan_id. Explicitly setting the type for javascript as a string will prevent these issues, and should not interfere with its use as an identifier.

I've run into this issue in a javascript project, and making these changes to the proto file manually was the solution. I figure this change would save the headache for others who may run into this as well down the road.

@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour rpc Related to the RPC interface P3 might get fixed, nice to have labels Dec 21, 2018
@sangaman sangaman closed this Apr 27, 2019
@sangaman sangaman deleted the jstype-string branch April 27, 2019 10:25
@ghost ghost mentioned this pull request Jun 17, 2019
Merged
@sangaman sangaman restored the jstype-string branch June 18, 2019 08:13
@sangaman sangaman reopened this Jun 18, 2019
@sangaman
Copy link
Contributor Author

I reopened this branch/PR and rebased onto master (including adding the jstype attribute in a few more places where channel id fields are used). This definitely seems helpful to have in the main repo as it would prevent bugs in javascript code that uses an unmodified version of the proto definitions.

@sangaman sangaman mentioned this pull request Jun 18, 2019
Closed
@halseth
Copy link
Contributor

halseth commented Jul 8, 2019

cc @tanx any thoughts on this?

@ghost
Copy link

ghost commented Aug 25, 2019

Bump.

@tanx
Copy link

tanx commented Aug 26, 2019

cc @tanx any thoughts on this?

We don't use chan_id in the app. So no strong feelings here. Casting to a string makes sense to prevent errors when dealing with 64 bit integers.

@sangaman
Copy link
Contributor Author

Let me know if you agree that this would be worth adding to the codebase and I'll rebase this PR, thanks.

@alexbosworth alexbosworth mentioned this pull request Aug 29, 2019
Closed
halseth
halseth approved these changes Sep 2, 2019
View reviewed changes
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 after rebase and proto-regen 👍

@sangaman
Copy link
Contributor Author

Sorry only seeing the approval now! Rebasing & regenerating protos now.

@sangaman
Copy link
Contributor Author

@halseth Rebased and searched all proto files for chan_id and channel_id to make sure they have the JS_STRING option set.

@halseth
Copy link
Contributor

halseth commented Sep 25, 2019

Need another rebase 😅

@sangaman sangaman force-pushed the jstype-string branch 2 times, most recently from 45948a8 to ebdb393 Compare September 25, 2019 17:26
@sangaman
Copy link
Contributor Author

@halseth done!

@sangaman
Copy link
Contributor Author

sangaman commented Oct 1, 2019

@halseth Rebased again.

cfromknecht
cfromknecht approved these changes Oct 3, 2019
View reviewed changes
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! Needs oneee last rebase 😅

@sangaman
Copy link
Contributor Author

sangaman commented Oct 7, 2019

@cfromknecht @halseth All your rebase are belong to us.

@sangaman
Copy link
Contributor Author

Re-rebased.

@sangaman
lnrpc: set JS_STRING option for chan_id
f437b35 
This sets the `jstype` option to `JS_STRING` for all `chan_id` fields
in the proto rpc definition. `chan_id` is a 64 bit integer, which is
not natively supported by javascript's floating-point `number` with
only 52 bit precision. Nevertheless, by default protobuf will use the
`number` type for 64 bit integer fields in javascript, which can cause
loss of precision problems with `chan_id`. Explicitly setting the type
for javascript as a string will prevent these issues, and should not
interfere with its use as an identifier.
@cfromknecht cfromknecht merged commit 7408fe5 into lightningnetwork:master Oct 16, 2019
@sangaman sangaman deleted the jstype-string branch October 16, 2019 11:04
@guggero
Copy link
Collaborator

guggero commented Nov 13, 2019

@sangaman could you tell us which JS library you are using?
It looks like this setting doesn't really have any impact with either grpc or @grpc/grpc-js.
But rather setting { longs: String } in the @grpc/proto-loader options seemed to fix this.

@sangaman
Copy link
Contributor Author

I'm using the grpc package on npm, but I think the main factor there is what you are using for protobuf.

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

@halseth halseth halseth approved these changes

@cfromknecht cfromknecht cfromknecht approved these changes

Assignees
No one assigned
Labels
enhancement Improvements to existing features / behaviour P3 might get fixed, nice to have rpc Related to the RPC interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sangaman @halseth @tanx @guggero @cfromknecht @Roasbeef