wallet encapsulation #2130

pull mikegogulski wants to merge 14 commits into bitcoin:master from mikegogulski:walletencap3 changing 6 files +195 −192

mikegogulski commented at 8:45 PM on December 26, 2012: none

hopefully a proper rebasing of #2075

The goal here is to work toward a clean interface to the wallet object. For now this involves moving code out of rpc*.cpp which deals with wallet internals and making that code into methods on the CWallet object.
Just some comments on what I'm aiming to do here 2ec0d5f0b4
more TODO comments for things to be moved to rpchelpers.cpp dc90bea8ad
Make GetAccountAddress and SetAccount into methods of CWallet f08745e910
move some stuff touched by setaccount into methods of CWallet 4e32558e14
Some touch-ups per Diapolo's diff comments. 539e241d81
make GetAccountBalance a method of CWallet; clean up some spacing 6f8f35bda8
make GetAccountAddresses into a method of CWallet d4c4aadb7b
snip an unneeded comment; inline a couple of variables in
CWallet::GetAccountAddresses()
981ff1fc12
use CWallet::GetAccountAddresses() in getaddressesbyaccount() 79fd78ad2c
indentation fix 15cf6f3db5
Remove CBitcoinAddress deps in wallet.cpp; Remove an inappropriate TODO. 603970d71e
replace CWallet::IsMine(CBitcoinAddress&) with
CWallet::IsMine(CTxDestination&)
cf9949e191
RPC commands now no longer depend on walletdb.h/CWalletDB 943e7768f3
in src/wallet.cpp:None in 603970d71e outdated
```
1857 | +    }
1858 | +
1859 | +    return SetAddressBookName(dest, strAccount);
1860 | +}
1861 | +
1862 | +bool CWallet::IsMine(const CBitcoinAddress& address) const
```
sipa commented at 2:05 PM on December 27, 2012:

Here's still a CBitcoinAddress left.

mikegogulski commented at 2:07 PM on December 27, 2012:

I need to hunt that one down yet since I think it's used outside of rpc*.cpp. I'll post a new commit working on that soon.
BitcoinPullTester commented at 8:01 AM on January 3, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/943e7768f33a7c766d1be5db8aea64a46e37253b for binaries and test log.
in src/rpcwallet.cpp:None in 943e7768f3 outdated
```
 169 | -            GetAccountAddress(strOldAccount, true);
 170 | -    }
 171 | -
 172 | -    pwalletMain->SetAddressBookName(address.Get(), strAccount);
 173 | -
 174 | +    // TODO: Use the return code for something
```
sipa commented at 12:27 AM on January 6, 2013:

Why is this a TODO?

mikegogulski commented at 12:38 AM on January 6, 2013:

I saw a method returning bool and the return value being ignored, so I put in that note. Looking around the codebase now I see that it's hardly ever used, so maybe it can be ignored. Want a new commit?
sipa commented at 12:43 AM on January 6, 2013: member

Many of your TODO's look like todo's for yourself rather than concrete plans for changes in the source code - leave those out. If there is a concrete plan, and it's obvious: just add a commit that actually implements the change. If it's something big you'd rather leave for a follow-up pull request, a TODO in the code is fine.

Also, squash some commits together. "Indentation fix" certainly doesn't require a separate commit.
Removed all my note-to-self TODOs 4debb8059a
mikegogulski commented at 1:00 AM on January 6, 2013: none

Okay, I stripped out the todos. Should I squash everything in this branch down to one commit in order to get it pulled? (total git noob here)
BitcoinPullTester commented at 1:17 AM on January 6, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/4debb8059a3d29a88b72b2bc61a4e0f6bfc7e522 for binaries and test log.
BitcoinPullTester commented at 4:38 AM on January 24, 2013: none

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/4debb8059a3d29a88b72b2bc61a4e0f6bfc7e522 for test log.

This pull does not merge cleanly onto current master
luke-jr commented at 6:36 PM on January 30, 2013: member

Rebase needed
jgarzik commented at 6:27 PM on June 19, 2013: contributor

Poke @mikegogulski

The general sentiment towards these changes seems positive. Let's rebase and get this moving, or close.
luke-jr commented at 4:09 AM on July 17, 2013: member

I've rebased this as my mikegogulski_walletencap3 branch. It does, however, create a dependency from wallet.cpp -> main.h; I don't see any obvious way to fix this, considering that IsFinalTx is not tied to any class.
jgarzik commented at 3:25 AM on August 25, 2013: contributor

Closing - non-responsive. Feel free to rebase and reopen.
jgarzik closed this on Aug 25, 2013
owlhooter referenced this in commit 97b9b4fedc on Oct 11, 2018
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#2075 Wallet encapsulation: TODOs, and make SetAddress and GetAccountAddress into methods of CWallet