Refactoring to create a 'wallet' package #213
Conversation
|
Please don't add a global wallet instance to the main package. You can end up with races between reading and setting the pointer. Instead, use the wallet pointers that are passed down to all of the handlers, after the wallet has been loaded and added to the server with |
| @@ -25,11 +25,13 @@ import ( | |||
| "runtime" | |||
|
|
|||
| "github.com/btcsuite/btcwallet/chain" | |||
| "github.com/mirrorx/btcwallet/wallet" | |||
There was a problem hiding this comment.
I'll change this at the very end - right before it's ready to merge.
|
This removes the global wallet ref |
| @@ -263,7 +255,7 @@ func loadConfig() (*config, []string, error) { | |||
| // Pre-parse the command line options to see if an alternative config | |||
| // file or the version flag was specified. | |||
| preCfg := cfg | |||
There was a problem hiding this comment.
This has changed semantics now because cfg is a pointer, not a config, so it doesn't make a shallow copy of the struct. Is this intentional?
There was a problem hiding this comment.
changed it back to how it was originally
|
In general, I'd like to see everything dealing with the creation of the wallet database and where it lives completely moved out of the new package. It would be much cleaner to leave it up to the caller where to locate the database file, when to create a new one, etc. The wallet package, imo, should just take a I think it will allow much more flexibility for direct integration into GUIs, running as a service, running as a daemon, etc. |
| dbPath := filepath.Join(netdir, walletDbName) | ||
| // OpenWallet opens a wallet from disk. | ||
| func OpenWallet(config *Config) (*Wallet, error) { | ||
| netdir := NetworkDir(config.DataDir, config.ChainParams) |
There was a problem hiding this comment.
As mentioned elsewhere, I'd prefer all of this be up to the caller and this function takes an already opened walletdb.DB to work with.
|
I don't think passing just an opened DB to the wallet package is enough at the moment, since txstore is outside the db. But we could pass both in, and then when wtxmgr goes live, introduce a breaking API change which removes the txstore option. I do find that cleaner than making the wallet package aware of the database file name. What I don't want is to have both the wallet file name and the data directory configurable, since that is madness. |
|
Agreed for btcwallet. By making the caller responsible for the db, that is a choice we can make for btcwallet. On the other hand, perhaps GUIs might want to support multiple wallets in the same directory (once txstore is moved into the db, this would be trivial as it's a single file), one wallet per directory across multiple directories, or some combination thereof. Making the caller responsible for such things offers the flexibility to do all of them. Having the wallet package responsible for it like it is now doesn't. Further, it allows the caller to create their own namespace and store data in the same database. For example, the GUI could save its state, options, etc. This type of stuff was the intent of designing walletdb with namespaces. It makes it trivial to add a new package/namespace and store whatever you like without having to worry about messing up any data stored by other packages. |
|
Moved out wallet files creation as well as ownership outside the wallet package. The wallet database and txStore need to be passed in as parameters to wallet.Open |
| @@ -0,0 +1,31 @@ | |||
| wallet | |||
| ======== | |||
There was a problem hiding this comment.
Too many =. The other READMEs make these match.
| return &db, nil | ||
| } | ||
|
|
||
| func openWaddrmgr(db *walletdb.DB, namespaceKey []byte, pass string, |
There was a problem hiding this comment.
Please comment the intent, caveats, etc of this function.
|
Also, please create a |
|
All changes reviewed. As discussed in IRC, please add TODOs in |
This a refactor of the btcwallet main package to create a new wallet package. The main feature of this package is the integration of all the other wallet components (waddrmgr, txstore, and chain) and the Wallet type is 'runnable', so it will be continuously updating itself against changes notified by the remote btcd instance. It also includes several methods which provide access to information necessary to run a wallet RPC server.
Travis build seems good, ready to be merged in. |
Needs dcrd commit b58d1c4bd45c3fce89318ada51dd1288b5c5c58d Fixes btcsuite#213
Needs dcrd commit b58d1c4bd45c3fce89318ada51dd1288b5c5c58d Fixes btcsuite#213
Needs dcrd commit b58d1c4bd45c3fce89318ada51dd1288b5c5c58d Fixes btcsuite#213
No description provided.