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

proto: deprecate duplicate string fields in requests #3661

Merged
merged 1 commit into from Nov 7, 2019

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Nov 1, 2019

Follow-up for #3650.

With this PR we deprecate fields that have been specifically added to
to work around a bug in the gRPC/REST gateway that didn't allow bytes
fields to be encoded in REST requests.
That bug has now been fixed so the fields are no longer required.
To make it more clear how bytes fields have to be used in REST,
comments have been added to all those fields.

@guggero guggero requested a review from joostjager Nov 1, 2019
@halseth halseth added this to the 0.9.0 milestone Nov 1, 2019
@halseth halseth added this to Needs Review in v0.9.0-beta Nov 1, 2019
Copy link
Collaborator

@joostjager joostjager left a comment

I think you looked for all the duplicated fields in this PR? I am wondering, what to do with the fields for which there is only a string field, which should really have been bytes? There are for example numerous pubkey fields.

lnrpc/rpc.proto Outdated
Hex-encoded string representing the funding transaction. Deprecated now
that the REST gateway supports base64 encoding of bytes fields.
*/
string funding_txid_str = 2 [json_name = "funding_txid_str", deprecated = true];
Copy link
Collaborator

@joostjager joostjager Nov 1, 2019

Choose a reason for hiding this comment

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

This makes the whole oneof redundant. But maybe that removal should be done in a second step?

Copy link
Collaborator Author

@guggero guggero Nov 6, 2019

Choose a reason for hiding this comment

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

I removed the deprecation from this field again. This is the case @Roasbeef mentioned where it might be useful for the users to be able to just pass in the string representation of a TXID that they get from a previous output and don't need to byte-reverse it before feeding it into the RPC/REST call. So the oneof can also remain.

@guggero
Copy link
Collaborator Author

@guggero guggero commented Nov 6, 2019

I removed the deprecation from the TXID field. There are only 5 deprecations in this PR now, all of them are duplicates and very likely introduced to work around the bug that is now fixed.
In my opinion we should not look at any other string fields that could be sent as bytes just yet. Changing all of those would introduce too many compatibility issues between versions.

@guggero guggero requested review from joostjager and wpaulino Nov 6, 2019
Copy link
Collaborator

@wpaulino wpaulino left a comment

I removed the deprecation from this field again. This is the case @Roasbeef mentioned where it might be useful for the users to be able to just pass in the string representation of a TXID that they get from a previous output and don't need to byte-reverse it before feeding it into the RPC/REST call. So the oneof can also remain.

Seems more like an inconsistency with our API then? As long as we accept and return them in the same format, there's no need to keep the string field.

lnrpc/rpc.proto Outdated
@@ -94,13 +94,15 @@ service WalletUnlocker {
message GenSeedRequest {
/**
aezeed_passphrase is an optional user provided passphrase that will be used
to encrypt the generated aezeed cipher seed.
to encrypt the generated aezeed cipher seed. When using REST, this field
Copy link
Collaborator

@wpaulino wpaulino Nov 6, 2019

Choose a reason for hiding this comment

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

Nit: there's a double space after the period here and in some other cases below that would be reflected in the API documentation.

Copy link
Collaborator Author

@guggero guggero Nov 7, 2019

Choose a reason for hiding this comment

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

The double space was used in several places so I thought that was a style we wanted to use for some reason. But I removed all of them now.

With this PR we deprecate fields that have been specifically added to
to work around a bug in the gRPC/REST gateway that didn't allow bytes
fields to be encoded in REST requests.
That bug has now been fixed so the fields are no longer required.
To make it more clear how bytes fields have to be used in REST,
comments have been added to all those fields.
@guggero guggero force-pushed the deprecate-rest-string branch from 203974e to 7e9c4f0 Compare Nov 7, 2019
@guggero
Copy link
Collaborator Author

@guggero guggero commented Nov 7, 2019

There are quite a few inconsistencies in the API, especially around the TXIDs.
For example, in the output the channel point is sometimes a string (<txid>:<idx>) and sometimes of type ChannelPoint. Which is in turn more or less the same as OutPoint.
Because this grew over time, there are many things that could be made cleaner.
Maybe we should do a massive API overhaul when we know how to proceed with the custom jsonpb library? It would be nice if we could switch everything over to hex encoding instead of base64 in REST.
But that sounds more like something for 0.10 or even 0.11.

Copy link
Collaborator

@wpaulino wpaulino left a comment

Because this grew over time, there are many things that could be made cleaner.
Maybe we should do a massive API overhaul when we know how to proceed with the custom jsonpb library?

Definitely, sounds good to me.

v0.9.0-beta automation moved this from Needs Review to Approved Nov 7, 2019
@wpaulino wpaulino merged commit b0dc3b7 into lightningnetwork:master Nov 7, 2019
2 checks passed
v0.9.0-beta automation moved this from Approved to Done Nov 7, 2019
@guggero guggero deleted the deprecate-rest-string branch Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v0.9.0-beta
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants