wallet.proto start #47
|
|
@ -14,12 +14,12 @@ message Wallet {
|
|||
}
|
||||
|
||||
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.
|
||||
message AddressManager { // Manager for deterministically generated addresses
|
||||
// TODO - it seems like chain can be as high as (1 << 32) - 1. I think that means that uint32 should cover `gap`?
|
||||
uint32 gap = 1; // Maximum allowed consecutive generated addresses with no transactions
|
||||
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
|
||||
message AddressManager { // Manager for deterministically generated addresses
|
||||
// TODO - it seems like chain can be as high as (1 << 32) - 1. I think that means that uint32 should cover `gap`?
|
||||
uint32 gap = 1; // Maximum allowed consecutive generated addresses with no transactions
|
||||
uint32 maximum_uses_per_address = 2 [json_name="maximum_uses_per_address"]; // Maximum number of uses for each generated address
|
||||
}
|
||||
string name = 1; // type of address generator: "deterministic-chain" or "single-address"
|
||||
|
@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?
just the two listed in the comment just the two listed in the comment
@orblivion can you make these @orblivion can you make these `oneOf`s plz
I thought 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.
Now, if this is valuable enough that schema changes are on the table, an enum may be the way to go. 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;
}
```
you're right, i meant enum you're right, i meant enum
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
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.
|
||||
AddressManager change = 2; // Manager for deterministically generated change address (not used if `name` is set to `"single-address")
|
||||
AddressManager receiving = 3; // Manager for deterministically generated receiving address (not used if `name` is set to `"single-address")
|
||||
|
|
|
|||
@eukreign can you take a look at the Account part of the protobuf and see if it looks right to you please?