novitski / bitcoinj

Automatically exported from code.google.com/p/bitcoinj
Apache License 2.0
0 stars 0 forks source link

Wallet.java too monolithic #230

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
This is a low priority enhancement request.  It overlaps issue 229.

It would be nice if the wallet could be assembled from  bits and pieces with 
maybe, a canned pre-assembled convenience version with similar functionality 
provided.

Right now it seems to be doing all of this:

1)Key store.
2)Transaction filter
3)Transaction store.
4)Transaction state listener.
5)Serializer
6)Transaction factory
7)Wallet migration tool.
8)Transaction verifier
9)Blockchain state listener.

1800 lines of code later it seems to me that perhaps it might be time for a 
reorg?

Original issue reported on code.google.com by mark.p.l...@gmail.com on 19 Jul 2012 at 6:48

GoogleCodeExporter commented 9 years ago
Yeah, definitely. The Wallet is a pretty central part of Bitcoin, but I think 
over time we'll refactor chunks out.

It hasn't happened yet because my focus is 100% on the mobile user experience 
and other apps are less of a priority. Also, the right refactorings to use 
aren't always clear until you see what other apps need and how they're doing it.

I think the right place to start is splitting out chunks that apps might 
reasonably want to customize. I have a branch that splits coin selection out 
into a separate class for this reason. It's a part of some fees work I was 
doing.

Original comment by hearn@google.com on 19 Jul 2012 at 9:00

GoogleCodeExporter commented 9 years ago

Original comment by hearn@google.com on 8 Jan 2013 at 4:41

GoogleCodeExporter commented 9 years ago
There's been some progress on this over time - code is slowly moving out to 
separated classes in the wallet package, and often getting more test coverage / 
becoming pluggable along the way. 

At the moment I'm working on carving out key management, but we've already done 
coin selection, risk analysis, file management and a few other smaller things.

Original comment by mh.in.en...@gmail.com on 24 Nov 2013 at 11:19

GoogleCodeExporter commented 9 years ago
Key management is now carved out into KeyChainGroup with the key fetching / 
encrypting / importing methods now just forwarding to that class, which in 
turns multiplexes DeterministicKeyChain and BasicKeyChain.

Unfortunately, the fight against Wallet.java is being lost pretty badly. It 
just passed 4000 lines of code, despite the above mentioned refactorings :( To 
some extent this is just because keeping the entrypoints around + javadocs ends 
up using lots of lines, even if the implementations are elsewhere. But it's 
also because the bulk of the Wallet functionality hasn't been truly split up 
yet.

Original comment by mh.in.en...@gmail.com on 2 Jun 2014 at 2:05

GoogleCodeExporter commented 9 years ago
Oh yes, also Bloom filter merging was also split into the FilterMerger class.

I think more splitting up may have to wait now until the 
Transaction->WalletPayment refactoring is done, as that could impact quite a 
lot of the code. After that it should be clearer how to break tracking of 
transaction (output) state into a separate class.

Original comment by mh.in.en...@gmail.com on 2 Jun 2014 at 2:07

GoogleCodeExporter commented 9 years ago
Just a thought: Wallet has some internal classes which I think would be better 
kept outside. E.g. SendRequest.

Original comment by andreas....@gmail.com on 2 Jun 2014 at 2:16