LBRYCommunity/types
1
0
Fork 0
Code Issues 13 Pull requests Packages Projects Releases Wiki Activity

wallet.proto start #47

Closed
orblivion wants to merge 14 commits from wallet.proto into master
pull from: wallet.proto
merge into: LBRYCommunity:master
Conversation 9 Commits 14 Files changed 3 +683
orblivion commented 2022-01-28 21:55:02 +01:00 (Migrated from github.com)
Copy link

@lyoshenka This is my first pass.

Using this, I tried to import the example_wallet file you gave me, and then export it back to json to compare the results. None of the values are different, but some fields are missing. Those fields are the ones set to default values (0, false, etc).

However, there's a parameter that can be set which explicitly includes those fields when exporting to json:

https://github.com/lbryio/types/compare/master...orblivion:wallet.proto?expand=1#diff-b0922d7aa57224bd677c3ef7062b30255ff2f74f138692b4163fe0533c0ff53eR17

If I set it to true and try it on the example wallet, I find that the example wallet was itself omitting a lot of fields that were set to defaults. The json export thus has more fields than the input example wallet. Additionally, the example wallet has language: null, and the json export has it set to empty string, since I assumed it was supposed to be a string value. I'm not sure what we should do about that.

There's also wallet_wrapper.py that handles the lbryum_servers conversion between [domain, port] (not protobuf-friendly) and {domain, port} aka DomainPortPair. I could add something for language null<->"" there if need be. And I'm sure many other fields like this.

So that's where I'm at. Currently I'm digging through the SDK to find evidence of other fields I may have missed.

  • I there anything else I should be doing to validate and iterate on this? How exactly will this be used? I suspect that there are many other variations in wallet files that could lead to issues.
  • Where are settings used? It looks like this repo doesn't reference specific settings.
  • Are there any secrets in the example wallet you gave? If not, I could put the before/after in a gist and post it here for reference.
@lyoshenka This is my first pass. Using this, I tried to import the example_wallet file you gave me, and then export it back to json to compare the results. None of the values are _different_, but some fields are missing. Those fields are the ones set to default values (0, false, etc). However, there's a parameter that can be set which explicitly includes those fields when exporting to json: https://github.com/lbryio/types/compare/master...orblivion:wallet.proto?expand=1#diff-b0922d7aa57224bd677c3ef7062b30255ff2f74f138692b4163fe0533c0ff53eR17 If I set it to true and try it on the example wallet, I find that the example wallet was itself omitting a lot of fields that were set to defaults. The json export thus has _more_ fields than the input example wallet. Additionally, the example wallet has `language: null`, and the json export has it set to empty string, since I assumed it was supposed to be a string value. I'm not sure what we should do about that. There's also `wallet_wrapper.py` that handles the `lbryum_servers` conversion between `[domain, port]` (not protobuf-friendly) and `{domain, port}` aka `DomainPortPair`. I could add something for `language` `null`<->`""` there if need be. And I'm sure many other fields like this. So that's where I'm at. Currently I'm digging through the SDK to find evidence of other fields I may have missed. * I there anything else I should be doing to validate and iterate on this? How exactly will this be used? I suspect that there are many other variations in wallet files that could lead to issues. * Where are settings used? It looks like this repo doesn't reference specific settings. * Are there any secrets in the example wallet you gave? If not, I could put the before/after in a gist and post it here for reference.
orblivion commented 2022-01-28 23:34:43 +01:00 (Migrated from github.com)
Copy link

Just poking around I found this function that migrates from an older format of the wallet:

9e43060d41/lbry/wallet/manager.py (L145-L150)

It looks like the account struct had something called seed_version, which (per history of account.py) seems to have been removed from the struct, and version which seems to have never been part of it.

Are these safe to leave out of my definition? (And maybe we should remove them from this migration function)?

Overall I'm concluding that I should just look at what's referred to by the to_dict and from_dict functions of the various classes.

Just poking around I found this function that migrates from an older format of the wallet: https://github.com/lbryio/lbry-sdk/blob/9e43060d412697783140e396768fb0f1e0a2de15/lbry/wallet/manager.py#L145-L150 It looks like the `account` struct had something called `seed_version`, which (per history of `account.py`) seems to have been removed from the struct, and `version` which seems to have never been part of it. Are these safe to leave out of my definition? (And maybe we should remove them from this migration function)? Overall I'm concluding that I should just look at what's referred to by the `to_dict` and `from_dict` functions of the various classes.
orblivion commented 2022-01-29 00:36:19 +01:00 (Migrated from github.com)
Copy link

From inspecting the sdk code I think I have Accounts covered. The big one now is preferences. Other than encrypt-on-disk, this repo doesn't refer to any specific preference. I'd need to know what repos use the preferences to confirm this part of the schema.

I'll move on to commenting the individual fields in Accounts for now.

From inspecting the sdk code I think I have Accounts covered. The big one now is preferences. Other than `encrypt-on-disk`, this repo doesn't refer to any specific preference. I'd need to know what repos use the preferences to confirm this part of the schema. I'll move on to commenting the individual fields in Accounts for now.
lyoshenka (Migrated from github.com) reviewed 2022-01-31 23:42:35 +01:00
lyoshenka (Migrated from github.com) left a comment
Copy link

this looks good. given our convo today, could you change preferences to be an open-ended field that the app can use how it wants? you can prolly drop the wallet_wrapper code as well since its all for preferences.

this looks good. given our convo today, could you change preferences to be an open-ended field that the app can use how it wants? you can prolly drop the wallet_wrapper code as well since its all for preferences.
v2/proto/wallet.proto
@ -0,0 +20,4 @@
uint32 maximum_uses_per_address = 2 [json_name="maximum_uses_per_address"]; // Maximum number of uses for each generated address
}
message AddressGenerator { // Meta-manager for both singular or deterministically generated addresses
string name = 1; // type of address generator: "deterministic-chain" or "single-address"
lyoshenka (Migrated from github.com) commented 2022-01-31 23:39:53 +01:00
Copy link

@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 (Migrated from github.com) reviewed 2022-01-31 23:44:57 +01:00
v2/proto/wallet.proto
@ -0,0 +20,4 @@
uint32 maximum_uses_per_address = 2 [json_name="maximum_uses_per_address"]; // Maximum number of uses for each generated address
}
message AddressGenerator { // Meta-manager for both singular or deterministically generated addresses
string name = 1; // type of address generator: "deterministic-chain" or "single-address"
eukreign (Migrated from github.com) commented 2022-01-31 23:44:57 +01:00
Copy link

just the two listed in the comment

just the two listed in the comment
orblivion commented 2022-02-01 04:57:16 +01:00 (Migrated from github.com)
Copy link

Interestingly there's something called google.protobuf.Struct that does some magic with oneof that I don't really understand. But that did the trick. This probably also means that there must have been a way to make just the list of arbitrary types for lbryum_servers without the wrapper.

However, it's not exactly the same coming in and out of protobuf:

145c137
<                     "app_welcome_version": 1,
---
>                     "app_welcome_version": 1.0,
157c149
<                             "updatedAt": 1631117369
---
>                             "updatedAt": 1631117369.0
168c160
<                             "updatedAt": 1627495168
---
>                             "updatedAt": 1627495168.0
172c164
<                     "editedCollections": {},
---
>                     "editedCollections": null,
228c220
<                                 50001
---
>                                 50001.0
232c224
<                                 50001
---
>                                 50001.0
268c260
<                             "updatedAt": 1623354433
---
>                             "updatedAt": 1623354433.0

This could happen with any arbitrary data with any app out there using the sdk, right? Seems like it'll probably explode in unpredictable ways.

Interestingly there's something called `google.protobuf.Struct` that does some magic with `oneof` that I don't really understand. But that did the trick. This probably also means that there must have been a way to make just the list of arbitrary types for `lbryum_servers` without the wrapper. However, it's not exactly the same coming in and out of protobuf: ``` 145c137 < "app_welcome_version": 1, --- > "app_welcome_version": 1.0, 157c149 < "updatedAt": 1631117369 --- > "updatedAt": 1631117369.0 168c160 < "updatedAt": 1627495168 --- > "updatedAt": 1627495168.0 172c164 < "editedCollections": {}, --- > "editedCollections": null, 228c220 < 50001 --- > 50001.0 232c224 < 50001 --- > 50001.0 268c260 < "updatedAt": 1623354433 --- > "updatedAt": 1623354433.0 ``` This could happen with any arbitrary data with any app out there using the sdk, right? Seems like it'll probably explode in unpredictable ways.
lyoshenka (Migrated from github.com) reviewed 2022-02-01 18:34:35 +01:00
v2/proto/wallet.proto
@ -0,0 +20,4 @@
uint32 maximum_uses_per_address = 2 [json_name="maximum_uses_per_address"]; // Maximum number of uses for each generated address
}
message AddressGenerator { // Meta-manager for both singular or deterministically generated addresses
string name = 1; // type of address generator: "deterministic-chain" or "single-address"
lyoshenka (Migrated from github.com) commented 2022-02-01 18:34:35 +01:00
Copy link

@orblivion can you make these oneOfs plz

@orblivion can you make these `oneOf`s plz
lyoshenka (Migrated from github.com) reviewed 2022-02-01 18:35:12 +01:00
v2/proto/wallet.proto
@ -0,0 +13,4 @@
uint32 version = 4; // Wallet spec version // TODO string? uint32?
}
message Account {
lyoshenka (Migrated from github.com) commented 2022-02-01 18:35:12 +01:00
Copy link

@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?
orblivion (Migrated from github.com) reviewed 2022-02-01 21:39:23 +01:00
v2/proto/wallet.proto
@ -0,0 +20,4 @@
uint32 maximum_uses_per_address = 2 [json_name="maximum_uses_per_address"]; // Maximum number of uses for each generated address
}
message AddressGenerator { // Meta-manager for both singular or deterministically generated addresses
string name = 1; // type of address generator: "deterministic-chain" or "single-address"
orblivion (Migrated from github.com) commented 2022-02-01 21:39:23 +01:00
Copy link

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 (Migrated from github.com) reviewed 2022-02-01 22:00:24 +01:00
v2/proto/wallet.proto
@ -0,0 +20,4 @@
uint32 maximum_uses_per_address = 2 [json_name="maximum_uses_per_address"]; // Maximum number of uses for each generated address
}
message AddressGenerator { // Meta-manager for both singular or deterministically generated addresses
string name = 1; // type of address generator: "deterministic-chain" or "single-address"
orblivion (Migrated from github.com) commented 2022-02-01 22:00:24 +01:00
Copy link

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; } ```
orblivion commented 2022-02-02 01:23:20 +01:00 (Migrated from github.com)
Copy link

lbry-sdk schema README specified a version of protoc so I downgraded and tried again. The Python is different, but the json round trip test is the same.

lbry-sdk schema README specified a version of protoc so I downgraded and tried again. The Python is different, but the json round trip test is the same.
eukreign (Migrated from github.com) reviewed 2022-02-02 03:40:17 +01:00
v2/proto/wallet.proto
@ -0,0 +13,4 @@
uint32 version = 4; // Wallet spec version // TODO string? uint32?
}
message Account {
eukreign (Migrated from github.com) commented 2022-02-02 03:40:16 +01:00
Copy link

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:43:25 +01:00 (Migrated from github.com)
Copy link

A quick note on the format change: The reason ints are changing to floats is that the `google.protobuf.Struct" message only handles doubles. Apparently that's in accordance with the json standard. One option would be to fork google's stuff and make it work with ints as well. However that would take time (proto definitions, and libraries for any language we use it in), and I don't know whether this is actually a problem we need to fix.

A quick note on the format change: The reason ints are changing to floats is that the `google.protobuf.Struct" message only handles doubles. Apparently that's in accordance with the json standard. One option would be to fork google's stuff and make it work with ints as well. However that would take time (proto definitions, and libraries for any language we use it in), and I don't know whether this is actually a problem we need to fix.
orblivion (Migrated from github.com) reviewed 2022-02-02 20:49:15 +01:00
v2/proto/wallet.proto
@ -0,0 +13,4 @@
uint32 version = 4; // Wallet spec version // TODO string? uint32?
}
message Account {
orblivion (Migrated from github.com) commented 2022-02-02 20:49:15 +01:00
Copy link

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 (Migrated from github.com) reviewed 2022-02-03 23:50:54 +01:00
v2/proto/wallet.proto
@ -0,0 +13,4 @@
uint32 version = 4; // Wallet spec version // TODO string? uint32?
}
message Account {
eukreign (Migrated from github.com) commented 2022-02-03 23:50:54 +01:00
Copy link

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 (Migrated from github.com) reviewed 2022-02-04 16:32:34 +01:00
v2/proto/wallet.proto
@ -0,0 +13,4 @@
uint32 version = 4; // Wallet spec version // TODO string? uint32?
}
message Account {
orblivion (Migrated from github.com) commented 2022-02-04 16:32:34 +01:00
Copy link

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 (Migrated from github.com) reviewed 2022-02-04 18:50:58 +01:00
v2/proto/wallet.proto
@ -0,0 +13,4 @@
uint32 version = 4; // Wallet spec version // TODO string? uint32?
}
message Account {
orblivion (Migrated from github.com) commented 2022-02-04 18:50:58 +01:00
Copy link

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 (Migrated from github.com) reviewed 2022-02-08 15:33:27 +01:00
v2/proto/wallet.proto
@ -0,0 +20,4 @@
uint32 maximum_uses_per_address = 2 [json_name="maximum_uses_per_address"]; // Maximum number of uses for each generated address
}
message AddressGenerator { // Meta-manager for both singular or deterministically generated addresses
string name = 1; // type of address generator: "deterministic-chain" or "single-address"
lyoshenka (Migrated from github.com) commented 2022-02-08 15:33:27 +01:00
Copy link

you're right, i meant enum

you're right, i meant enum
lyoshenka (Migrated from github.com) reviewed 2022-02-08 15:34:39 +01:00
v2/proto/wallet.proto
@ -0,0 +20,4 @@
uint32 maximum_uses_per_address = 2 [json_name="maximum_uses_per_address"]; // Maximum number of uses for each generated address
}
message AddressGenerator { // Meta-manager for both singular or deterministically generated addresses
string name = 1; // type of address generator: "deterministic-chain" or "single-address"
lyoshenka (Migrated from github.com) commented 2022-02-08 15:34:39 +01:00
Copy link

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
lyoshenka (Migrated from github.com) reviewed 2022-02-08 15:41:24 +01:00
v2/proto/wallet.proto
@ -0,0 +13,4 @@
uint32 version = 4; // Wallet spec version // TODO string? uint32?
}
message Account {
lyoshenka (Migrated from github.com) commented 2022-02-08 15:41:24 +01:00
Copy link

@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 (Migrated from github.com) reviewed 2022-02-08 15:41:41 +01:00
v2/proto/wallet.proto
@ -0,0 +13,4 @@
uint32 version = 4; // Wallet spec version // TODO string? uint32?
}
message Account {
lyoshenka (Migrated from github.com) commented 2022-02-08 15:41:40 +01:00
Copy link

so given that, are you still against this pr?

so given that, are you still against this pr?
lyoshenka commented 2022-02-08 15:44:21 +01:00 (Migrated from github.com)
Copy link

The reason ints are changing to floats is that the `google.protobuf.Struct" message only handles doubles. Apparently that's in accordance with the json standard.

That's kinda strange. what do they recommend for large ints? Should we use a string?

> The reason ints are changing to floats is that the `google.protobuf.Struct" message only handles doubles. Apparently that's in accordance with the json standard. That's kinda strange. what do they recommend for large ints? Should we use a string?
lyoshenka commented 2022-02-08 15:46:06 +01:00 (Migrated from github.com)
Copy link

also if this is for the arbitrary data part, that doesn't need to be protobuffed. you can leave that whole thing generic -- maybe protobufs have a type for raw bytes or something else that means "anything can go here".

also if this is for the arbitrary data part, that doesn't need to be protobuffed. you can leave that whole thing generic -- maybe protobufs have a type for raw bytes or something else that means "anything can go here".
orblivion commented 2022-02-08 18:36:32 +01:00 (Migrated from github.com)
Copy link

Yeah I'll try bytes again (pending Lex's response), though I'm not totally sure how it would work. This is arbitrary json data, not arbitrary serialized data. I'm not sure what that would really mean in terms of protobuf, other than a) something like Struct that maintains the structure and converts each primitive type, which we tried, or b) the text representation of the json. If we go with the latter, we can't just stick the text into a field or we'd end up escaping it, like:

{
  "arbitrary-data": "{\"foo\": \"bar\"}"
}

But, I think google's special protobuf types like Struct already use conversion magic, built into the libraries for the different languages. Maybe I can make my own.

Yeah I'll try `bytes` again (pending Lex's response), though I'm not totally sure how it would work. This is arbitrary json data, not arbitrary serialized data. I'm not sure what that would really mean in terms of protobuf, other than a) something like `Struct` that maintains the structure and converts each primitive type, which we tried, or b) the text representation of the json. If we go with the latter, we can't just stick the text into a field or we'd end up escaping it, like: ``` { "arbitrary-data": "{\"foo\": \"bar\"}" } ``` But, I think google's special protobuf types like `Struct` already use conversion magic, built into the libraries for the different languages. Maybe I can make my own.
orblivion (Migrated from github.com) reviewed 2022-02-08 21:33:51 +01:00
v2/proto/wallet.proto
@ -0,0 +20,4 @@
uint32 maximum_uses_per_address = 2 [json_name="maximum_uses_per_address"]; // Maximum number of uses for each generated address
}
message AddressGenerator { // Meta-manager for both singular or deterministically generated addresses
string name = 1; // type of address generator: "deterministic-chain" or "single-address"
orblivion (Migrated from github.com) commented 2022-02-08 21:33:50 +01:00
Copy link

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.
orblivion commented 2022-02-08 21:46:45 +01:00 (Migrated from github.com)
Copy link

large ints ... as a string

Oh interesting point. Yeah I guess so, if we don't got the arbitrary json blob route.

> large ints ... as a string Oh interesting point. Yeah I guess so, if we don't got the arbitrary json blob route.
eukreign (Migrated from github.com) reviewed 2022-02-09 14:18:01 +01:00
v2/proto/wallet.proto
@ -0,0 +13,4 @@
uint32 version = 4; // Wallet spec version // TODO string? uint32?
}
message Account {
eukreign (Migrated from github.com) commented 2022-02-09 14:18:01 +01:00
Copy link

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 (Migrated from github.com) reviewed 2022-02-09 16:53:49 +01:00
v2/proto/wallet.proto
@ -0,0 +13,4 @@
uint32 version = 4; // Wallet spec version // TODO string? uint32?
}
message Account {
orblivion (Migrated from github.com) commented 2022-02-09 16:53:49 +01:00
Copy link

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 (Migrated from github.com) reviewed 2022-02-09 20:36:16 +01:00
v2/proto/wallet.proto
@ -0,0 +13,4 @@
uint32 version = 4; // Wallet spec version // TODO string? uint32?
}
message Account {
orblivion (Migrated from github.com) commented 2022-02-09 20:36:16 +01:00
Copy link

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.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
area: devops

   
area: discovery

   
area: docs

   
area: livestream

   
area: proposal

Work is part of a proposal

  
consider soon

Discuss this issue at the next planning meeting, then remove this label

  
Epic

   
good first issue

   
hacktoberfest

Welcome to Hacktoberfest

  
help wanted

   
icebox

Long-term storage

  
level: 1

No knowledge of the existing code required

  
level: 2

Some knowledge of the existing code is recommended

  
level: 3

Significant knowledge of the existing code is recommended

  
level: 4

Intimate knowledge of the existing code is recommended

  
needs: exploration

Solution unclear, needs research

  
needs: grooming

Issue needs to be groomed before work can start

  
needs: priority

Priority level needs to be defined

  
needs: repro

Issue needs step-by-step instructions on how to reproduce in latest code

  
needs: tech design

Needs technical design signoff before implementation can begin

  
on hold

Temporarily paused

  
priority: blocker

Issue is blocking release, do ASAP

  
priority: high

Work needs to be moved into sprint ASAP

  
priority: low

Work should be done but can stay in the backlog for now

  
priority: medium

Work needs to be done within 2-3 sprints

  
resilience

   
Tom's Wishlist

   
type: bug

Existing functionality is wrong or broken

  
type: discussion

A conversation. No specific changes requested

  
type: improvement

Existing (or partially existing) functionality needs to be changed

  
type: new feature

New functionality that does not exist yet

  
type: refactor

Minimal user-visible changes, but significant internal work

  
type: task

Either work that's not related to the code, or a small chore that does not fit into other categories

  
type: testing

Solution needs additional user testing

  
unplanned

Work that was not planned into the spirnt, took priority over planned work

No labels
area: devops
area: discovery
area: docs
area: livestream
area: proposal
consider soon
Epic
good first issue
hacktoberfest
help wanted
icebox
level: 1
level: 2
level: 3
level: 4
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
on hold
priority: blocker
priority: high
priority: low
priority: medium
resilience
Tom's Wishlist
type: bug
type: discussion
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/types#47
No description provided.
Delete branch "wallet.proto"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?