wallet.proto start #47

Closed
orblivion wants to merge 14 commits from wallet.proto into master
3 changed files with 1408 additions and 0 deletions
Showing only changes of commit aa0f671724 - Show all commits

139
v2/proto/wallet.proto Normal file
View file

@ -0,0 +1,139 @@
syntax = "proto3";
package pb;
// TODO - timestamps need not be uint64 right?
message Wallet {
repeated Account accounts = 1;
string name = 2;
TimestampedPreferences preferences = 3;
uint32 version = 4;
/*
Python had these but probably unrelated
self.storage = storage or WalletStorage()
lyoshenka commented 2022-02-01 18:35:12 +01:00 (Migrated from github.com)
Review

@eukreign can you take a look at the Account part of the protobuf and see if it looks right to you please?

@eukreign can you take a look at the Account part of the protobuf and see if it looks right to you please?
eukreign commented 2022-02-02 03:40:16 +01:00 (Migrated from github.com)
Review

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
orblivion commented 2022-02-02 20:49:15 +01:00 (Migrated from github.com)
Review

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.
eukreign commented 2022-02-03 23:50:54 +01:00 (Migrated from github.com)
Review

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/

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/
orblivion commented 2022-02-04 16:32:34 +01:00 (Migrated from github.com)
Review

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.

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.
orblivion commented 2022-02-04 18:50:58 +01:00 (Migrated from github.com)
Review

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
lyoshenka commented 2022-02-08 15:41:24 +01:00 (Migrated from github.com)
Review

@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.
lyoshenka commented 2022-02-08 15:41:40 +01:00 (Migrated from github.com)
Review

so given that, are you still against this pr?

so given that, are you still against this pr?
eukreign commented 2022-02-09 14:18:01 +01:00 (Migrated from github.com)
Review

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.

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.
orblivion commented 2022-02-09 16:53:49 +01:00 (Migrated from github.com)
Review

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.

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.
orblivion commented 2022-02-09 20:36:16 +01:00 (Migrated from github.com)
Review

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.
self.encryption_password = None
self.id = self.get_id()
*/
}
message Account {
message AddressGenerator {
lyoshenka commented 2022-01-31 23:39:53 +01:00 (Migrated from github.com)
Review

@eukreign can this name field contain any name or is it choosing from a fixed set of options?

@eukreign can this name field contain any name or is it choosing from a fixed set of options?
eukreign commented 2022-01-31 23:44:57 +01:00 (Migrated from github.com)
Review

just the two listed in the comment

just the two listed in the comment
lyoshenka commented 2022-02-01 18:34:35 +01:00 (Migrated from github.com)
Review

@orblivion can you make these oneOfs plz

@orblivion can you make these `oneOf`s plz
orblivion commented 2022-02-01 21:39:23 +01:00 (Migrated from github.com)
Review

I thought oneof was for fields (and thus types), not values. The only thing I saw for values was an enum, and that's only integers.

I thought `oneof` was for fields (and thus types), not values. The only thing I saw for values was an enum, and that's only integers.
orblivion commented 2022-02-01 22:00:24 +01:00 (Migrated from github.com)
Review

Now, if this is valuable enough that schema changes are on the table, an enum may be the way to go.

enum AddressManagerType {
  single_address = 0;
  deterministic_chain = 1;
}
Now, if this is valuable enough that schema changes are on the table, an enum may be the way to go. ``` enum AddressManagerType { single_address = 0; deterministic_chain = 1; } ```
lyoshenka commented 2022-02-08 15:33:27 +01:00 (Migrated from github.com)
Review

you're right, i meant enum

you're right, i meant enum
lyoshenka commented 2022-02-08 15:34:39 +01:00 (Migrated from github.com)
Review

if enum doesn't do strings, there are two options. one is to leave it as a string. the other is to have it as an enum but then convert that to a string on save/load.

i think leaving it as a string is fine here

if enum doesn't do strings, there are two options. one is to leave it as a string. the other is to have it as an enum but then convert that to a string on save/load. i think leaving it as a string is fine here
orblivion commented 2022-02-08 21:33:50 +01:00 (Migrated from github.com)
Review

If we need to do custom conversion code for the preferences json blob, I could do that here too.

If we need to do custom conversion code for the preferences json blob, I could do that here too.
message Chain { // TODO - good name?
uint32 gap = 1; // uint64? not sure how big this might get
uint32 maximum_uses_per_address = 2 [json_name="maximum_uses_per_address"];
}
Chain change = 1;
string name = 2;
Chain receiving = 3;
}
AddressGenerator address_generator = 1 [json_name="address_generator"];
map<string, string> certificates = 2;
bool encrypted = 3;
string ledger = 4;
uint32 modified_on = 5 [json_name="modified_on"];
string name = 6;
string private_key = 7 [json_name="private_key"];
string public_key = 8 [json_name="public_key"];
string seed = 9;
}
message TimestampedPreferences {
EnableSync enable_sync = 1 [json_name="enable-sync"];
Preferences local = 2;
Preferences shared = 3;
message EnableSync {
uint32 ts = 1;
bool value = 2;
}
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?
}
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;
}
}

1244
v2/python/wallet_pb2.py Normal file

File diff suppressed because one or more lines are too long

View file

@ -0,0 +1,25 @@
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)