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

Wallet.loadFromFile takes WalletExtensions to deserialize #286

Closed

Conversation

kmels
Copy link
Contributor

@kmels kmels commented Nov 29, 2014

When saving a wallet with an extension, i would like to get the wallet including the extension back without using parseToProto(input).

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 05f199e on kmels:load-wallet-file-using-extensions into * on bitcoinj:master*.

@mikehearn
Copy link
Member

You can already do this by just instantiating your own WalletProtobufSerializer, adding the extensions to the wallet and then deserializing into it. However we have no documentation beyond the javadocs for wallet extensions so I am not entirely surprised you don't know this. Also the API is rather confusing and could need a redesign. But is this the right design?

@kmels
Copy link
Contributor Author

kmels commented Nov 30, 2014

Yes I know, however I was rather surprised by how the following snippet seems idiomatic yet it throws an exception when the default deserializer doesn't consider extensions. I believe the API should support the inverse operation automatically, but the API used is Wallet and not WalletProtobufSerializer.

Wallet w = new Wallet(..);
File f = new File(..);
WalletExtension ext = ...
extendedWallet.addExtension(ext); //ext can be mandatory
extendedWallet.saveToFile(f);
Wallet wallet = Wallet.loadFromFile(f); //exception, serializer can't deserialize.

If we want to discuss right API then in my opinion a Wallet should contain a serializer (WalletProtobufSerializer) that contains supported extensions itself, and an instance of a wallet can be made from a file. The serializer should be a singleton but also be able to be overriden, not sure if this is possible.. perhaps i should implement another reader as you suggest. 👍

In my case I use scala and I don't extend the Wallet class, instead I have a metadata class that persists these extensions and helps me retrieve a Wallet that is consistent. By consistent I mean that wallet implies metadata (owner's checksum) and metadata verifies wallet. I know i am replicating but i am trying to "scale" different wallet files.



/** Returns a wallet deserialized from the given file. */
public static Wallet loadFromFile(File f) throws UnreadableWalletException { return loadFromFile(f, null); }
Copy link
Member

Choose a reason for hiding this comment

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

this method shouldn't be necessary, surely? loadFromFile(file) still works with the above prototype change. It's source (probably not binary) compatible.

@kmels
Copy link
Contributor Author

kmels commented Dec 2, 2014

Thanks for reviewing. I made some changes

@mikehearn
Copy link
Member

Merged and added you to the AUTHORS file, thanks!

@mikehearn mikehearn closed this Dec 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants