wallet.proto start #47
|
|
@ -2,13 +2,15 @@ syntax = "proto3";
|
|||
|
||||
package pb;
|
||||
|
||||
import "google/protobuf/struct.proto";
|
||||
|
||||
// TODO - timestamps need not be uint64 right?
|
||||
|
||||
message Wallet {
|
||||
repeated Account accounts = 1; // Accounts associated with this wallet
|
||||
string name = 2; // Human readable name for this wallet
|
||||
TimestampedPreferences preferences = 3;
|
||||
uint32 version = 4; // Wallet spec version
|
||||
uint32 version = 4; // Wallet spec version // TODO string? uint32?
|
||||
}
|
||||
|
||||
message Account {
|
||||
hard to give feedback since i don't know what this is all for and why we need it hard to give feedback since i don't know what this is all for and why we need it
In the grand scheme, we're working on a new non-custodial wallet sync system. That means we need to think about how often to sync, and thus what to sync. For that, we should have an inventory of what's in the wallet. And then we can talk about changing it if need be. And it's also useful in its own right. It serves as documentation for whoever wants it. But also, if we make a protobuf definition, we can use it in the SDK (I'm starting to poke at this right now as well). Anybody who wants to re-implement the SDK could use it. In the grand scheme, we're working on a new non-custodial wallet sync system. That means we need to think about how often to sync, and thus what to sync. For that, we should have an inventory of what's in the wallet. And then we can talk about changing it if need be.
And it's also useful in its own right. It serves as documentation for whoever wants it. But also, if we make a protobuf definition, we can use it in the SDK (I'm starting to poke at this right now as well). Anybody who wants to re-implement the SDK could use it.
The wallet is currently stored as JSON, which is already readable by pretty much every programming language and plain text tool out there (linux command line tools); not to mention readable by people via a text editor.
I would be against adding this to the SDK since I think the wallet should be in an easily accessible format but maybe your protobuf wallet version makes sense for your sync tool, I just don't know enough about your project to provide feedback. If you just want to document the current JSON wallet structure there is https://json-schema.org/ The wallet is currently stored as JSON, which is already readable by pretty much every programming language and plain text tool out there (linux command line tools); not to mention readable by people via a text editor.
`protobuf` is nice if you absolutely have to squeeze every last bit of bytes out of your storage medium (such storing data on blockchain) but in terms of debugging and interoperability it's somewhat more limited (not every language has protobuf implementation).
I would be against adding this to the SDK since I think the wallet should be in an easily accessible format but maybe your protobuf wallet version makes sense for your sync tool, I just don't know enough about your project to provide feedback.
If you just want to document the current JSON wallet structure there is https://json-schema.org/
Ah yes I made one thing unclear - I didn't mean that we would change the saved format. Just that we'd use the python pb libraries everywhere we could, but ultimately parse and generate json. Overall I think Grin's idea was to use Ah yes I made one thing unclear - I didn't mean that we would change the saved format. Just that we'd use the python pb libraries everywhere we could, but ultimately parse and generate json.
Overall I think Grin's idea was to use `types/` as our one repository of formats and add the wallet to it. But I'll let him make his case.
Here's my exploratory WIP, so you can see what it might look like: https://github.com/lbryio/lbry-sdk/compare/master...orblivion:wallet.proto?expand=1 Here's my exploratory WIP, so you can see what it might look like:
https://github.com/lbryio/lbry-sdk/compare/master...orblivion:wallet.proto?expand=1
@eukreign i agree with you that wallet files should stay json so ppl can read them easily (though this is not a strong preference). the reason i chose protobuf to document the structure is that we already use protobufs in a few places in our code and it would let us stay consistent. if i were choosing in a vacuum, id probably choose json-schema. @eukreign i agree with you that wallet files should stay json so ppl can read them easily (though this is not a strong preference). the reason i chose protobuf to document the structure is that we already use protobufs in a few places in our code and it would let us stay consistent. if i were choosing in a vacuum, id probably choose json-schema.
so given that, are you still against this pr? so given that, are you still against this pr?
The WIP appears to delete a bunch of The WIP appears to delete a bunch of `to_dict`, which I've found very useful during debugging when working on the SDK. I'm still not sure what benefit this would bring to the SDK. Is the wallet sync mentioned earlier going to be implemented in the SDK? I don't want to change something that is already working well unless there is a really good reason for it; especially something as important as wallet serialization. People will be upset if they lose their funds.
Sure, it was a blind first go at it. I wouldn't have removed I agree, I'd worry about making changes to stable code at the risk of losing funds. I think the original proposition was less about a benefit to the SDK, and more about long term uniformity and consistency of LBRY-related formats as defined in @lyoshenka - if that's right, with all that in mind perhaps I could approach the PR totally differently. If the goal is to make sure that the format defined in As I wrote it, I actually went out of my way to use the protobuf primitives. But the Python protobuf library accepts dicts as input. I could make the test pretty simple: Take the dict coming out of the wallet functions, convert it to a protobuf object, and then check the protobuf fields to make sure they match the values in the input dict. ...and then over time, as changes are made to that core functionality of the SDK, we could choose to start to introduce the protobuf definition more directly. Or not. As for whether the SDK is going to be touched for sync: Mostly it's about sending the wallet over the wire, which afaik (I'm still internalising the LBRY stack) is mostly what the app does. As I understand, the SDK is already handing the wallet to the app for the current wallet sync system. I was going to add some metadata, but I'm not sure if that's SDK related or app related. There will also be the issue of conflict resolutions, which may or may not be fundamentally new. I'm not sure if the SDK would be a better place for that since it handles the wallet format more directly. Sure, it was a blind first go at it. I wouldn't have removed `to_dict` if it occurred to me that it was useful for debuging. Maybe POC would be more accurate than WIP. It was also a chance to get a better sense of the code base, including by making mistakes like this.
I agree, I'd worry about making changes to stable code at the risk of losing funds.
I think the original proposition was less about a benefit to the SDK, and more about long term uniformity and consistency of LBRY-related formats as defined in `types`. Perhaps it could be at a short term expense of putting it into the lbry-sdk repository. Though I could throw in, perhaps post-hoc, that long term it might in principle benefit the sdk as an extra check to make sure that the format doesn't accidentally deviate as code changes are made.
@lyoshenka - if that's right, with all that in mind perhaps I could approach the PR totally differently. If the goal is to make sure that the format defined in `types` is the one used in the SDK, what if I _only_ write test cases to ensure this? Then I won't risk affecting the functionality at all. For the SDK's sake it would at worst be a small waste of development time, and at best catch some mistakes early.
As I wrote it, I actually went out of my way to use the protobuf primitives. But the Python protobuf library accepts dicts as input. I could make the test pretty simple: Take the dict coming out of the wallet functions, convert it to a protobuf object, and then check the protobuf fields to make sure they match the values in the input dict.
...and then over time, as changes are made to that core functionality of the SDK, we could choose to start to introduce the protobuf definition more directly. Or not.
As for whether the SDK is going to be touched for sync: Mostly it's about sending the wallet over the wire, which afaik (I'm still internalising the LBRY stack) is mostly what the app does. As I understand, the SDK is already handing the wallet to the app for the current wallet sync system. I was going to add some metadata, but I'm not sure if that's SDK related or app related. There will also be the issue of conflict resolutions, which may or may not be fundamentally new. I'm not sure if the SDK would be a better place for that since it handles the wallet format more directly.
From talking with @lyoshenka we decided json-schema makes most sense if only because the arbitrary json data in the preferences fields for the apps makes an awkward fit for protobuf. I'll close this issue, but we may reopen the question of integrating the json-schema to make sure the sdk conforms, analogously to what we've discussed here with protobuf. From talking with @lyoshenka we decided json-schema makes most sense if only because the arbitrary json data in the preferences fields for the apps makes an awkward fit for protobuf. I'll close this issue, but we may reopen the question of integrating the json-schema to make sure the sdk conforms, analogously to what we've discussed here with protobuf.
|
||||
|
|
@ -65,87 +67,10 @@ message TimestampedPreferences {
|
|||
message Preferences {
|
||||
message Preferences_ {
|
||||
string type = 1; // TODO 'object' -- anything else?
|
||||
Preferences__ value = 2;
|
||||
string version = 3; // TODO why is this a string but not CurrencyAmount.amount?
|
||||
google.protobuf.Struct value = 2;
|
||||
string version = 3; // TODO string? uint32?
|
||||
}
|
||||
uint32 ts = 1;
|
||||
Preferences_ value = 2;
|
||||
}
|
||||
message Preferences__ {
|
||||
message Collection {
|
||||
string id = 1;
|
||||
repeated string items = 2;
|
||||
string name = 3;
|
||||
string type = 4; // TODO - always playlist?
|
||||
uint32 updatedAt = 5;
|
||||
}
|
||||
message BuiltInCollections {
|
||||
Collection favorites = 1;
|
||||
Collection watchlater = 2;
|
||||
}
|
||||
message Following {
|
||||
bool notificationsDisabled = 1;
|
||||
string uri = 2;
|
||||
}
|
||||
message Settings { // TODO check relevant code for more values
|
||||
bool automatic_dark_mode_enabled = 1 [json_name="automatic_dark_mode_enabled"];
|
||||
bool autoplay = 2;
|
||||
bool autoplay_next = 3 [json_name="autoplay_next"];
|
||||
message Time {
|
||||
string formattedTime = 1;
|
||||
string hour = 2; // TODO - can this safely be a number instead? convert it during de/serialization?
|
||||
string min = 3;
|
||||
}
|
||||
message TimeRange {
|
||||
Time from = 1;
|
||||
Time to = 2;
|
||||
}
|
||||
TimeRange dark_mode_times = 4 [json_name="dark_mode_times"];
|
||||
bool floating_player = 5 [json_name="floating_player"];
|
||||
bool hide_balance = 6 [json_name="hide_balance"];
|
||||
bool hide_reposts = 7 [json_name="hide_reposts"];
|
||||
bool hide_splash_animation = 8 [json_name="hide_splash_animation"];
|
||||
bool instant_purchase_enabled = 9 [json_name="instant_purchase_enabled"];
|
||||
message CurrencyAmount {
|
||||
double amount = 1; // TODO decimal type via string? is this safe?
|
||||
string currency = 2;
|
||||
}
|
||||
CurrencyAmount instant_purchase_max = 10 [json_name="instant_purchase_max"];
|
||||
|
||||
// TODO - I'm guessing this is a string, but the example value was
|
||||
// `null`. What do we do about this? Can the sdk treat omission as
|
||||
// `null`, or should we put this into the json translation wrapper?
|
||||
string language = 11;
|
||||
|
||||
// TODO - this struct is represented as [string, uint32] in the wallet
|
||||
// files. We cannot represent this in protobuf. We will need an extra
|
||||
// step before/after json de/serialization:
|
||||
//
|
||||
// {domain: "sdk.lbry.tech", port: 50001} <-> ["sdk.lbry.tech", 50001]
|
||||
message DomainPortPair {
|
||||
string domain = 1; // TODO - better name?
|
||||
uint32 port = 2;
|
||||
}
|
||||
repeated DomainPortPair lbryum_servers = 12 [json_name="lbryum_servers"];
|
||||
|
||||
bool share_usage_data = 13 [json_name="share_usage_data"];
|
||||
bool show_mature = 14 [json_name="show_mature"];
|
||||
string theme = 15;
|
||||
}
|
||||
|
||||
// TODO - In the example wallet, this is a string under the local
|
||||
// preferences but a number under shared preferences. Which should we go
|
||||
// with? Probably string, in case we get decimal version numbers.
|
||||
uint32 app_welcome_version = 1 [json_name="app_welcome_version"];
|
||||
repeated string blocked = 2;
|
||||
BuiltInCollections builtinCollections = 3;
|
||||
repeated string coin_swap_codes = 4 [json_name="coin_swap_codes"]; // TODO - check the type
|
||||
map<string, Collection> editedCollections = 5;
|
||||
repeated Following following = 6;
|
||||
Settings settings = 7;
|
||||
bool sharing_3P = 8 [json_name="sharing_3P"];
|
||||
repeated string subscriptions = 9;
|
||||
repeated string tags = 10;
|
||||
map<string, Collection> unpublishedCollections = 11;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,25 +0,0 @@
|
|||
from google.protobuf.json_format import MessageToDict, ParseDict
|
||||
|
||||
import json
|
||||
|
||||
import wallet_pb2
|
||||
|
||||
def from_json(wallet_json, wallet_pb):
|
||||
wallet_dict = json.loads(wallet_json)
|
||||
settings = wallet_dict['preferences']['shared']['value']['value']['settings']
|
||||
settings['lbryum_servers'] = [
|
||||
{'domain': domain, 'port': port}
|
||||
for [domain, port]
|
||||
in settings['lbryum_servers']
|
||||
]
|
||||
return ParseDict(wallet_dict, wallet_pb)
|
||||
|
||||
def to_json(wallet_pb, including_default_value_fields):
|
||||
wallet_dict = MessageToDict(wallet_pb, including_default_value_fields=including_default_value_fields)
|
||||
settings = wallet_dict['preferences']['shared']['value']['value']['settings']
|
||||
settings['lbryum_servers'] = [
|
||||
[domain_port_pair['domain'], domain_port_pair['port']]
|
||||
for domain_port_pair
|
||||
in settings['lbryum_servers']
|
||||
]
|
||||
return json.dumps(wallet_dict)
|
||||
@eukreign can you take a look at the Account part of the protobuf and see if it looks right to you please?