wallet encapsulation #2130

Closed
wants to merge 14 commits into
from

Conversation

Projects
None yet
5 participants
@mikegogulski

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.

src/wallet.cpp
+ return SetAddressBookName(dest, strAccount);
+}
+
+bool CWallet::IsMine(const CBitcoinAddress& address) const

This comment has been minimized.

Show comment Hide comment
@sipa

sipa Dec 27, 2012

Owner

Here's still a CBitcoinAddress left.

@sipa

sipa Dec 27, 2012

Owner

Here's still a CBitcoinAddress left.

This comment has been minimized.

Show comment Hide comment
@mikegogulski

mikegogulski Dec 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.

@mikegogulski

mikegogulski Dec 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

This comment has been minimized.

Show comment Hide comment
@BitcoinPullTester

BitcoinPullTester Jan 3, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/943e7768f33a7c766d1be5db8aea64a46e37253b for binaries and test log.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/943e7768f33a7c766d1be5db8aea64a46e37253b for binaries and test log.

src/rpcwallet.cpp
-
- pwalletMain->SetAddressBookName(address.Get(), strAccount);
-
+ // TODO: Use the return code for something

This comment has been minimized.

Show comment Hide comment
@sipa

sipa Jan 6, 2013

Owner

Why is this a TODO?

@sipa

sipa Jan 6, 2013

Owner

Why is this a TODO?

This comment has been minimized.

Show comment Hide comment
@mikegogulski

mikegogulski Jan 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?

@mikegogulski

mikegogulski Jan 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

This comment has been minimized.

Show comment Hide comment
@sipa

sipa Jan 6, 2013

Owner

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.

Owner

sipa commented Jan 6, 2013

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.

@mikegogulski

This comment has been minimized.

Show comment Hide comment
@mikegogulski

mikegogulski Jan 6, 2013

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)

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

This comment has been minimized.

Show comment Hide comment
@BitcoinPullTester

BitcoinPullTester Jan 6, 2013

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

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

@BitcoinPullTester

This comment has been minimized.

Show comment Hide comment
@BitcoinPullTester

BitcoinPullTester Jan 24, 2013

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

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

This comment has been minimized.

Show comment Hide comment
@luke-jr

luke-jr Jan 30, 2013

Member

Rebase needed

Member

luke-jr commented Jan 30, 2013

Rebase needed

@jgarzik

This comment has been minimized.

Show comment Hide comment
@jgarzik

jgarzik Jun 19, 2013

Contributor

Poke @mikegogulski

The general sentiment towards these changes seems positive. Let's rebase and get this moving, or close.

Contributor

jgarzik commented Jun 19, 2013

Poke @mikegogulski

The general sentiment towards these changes seems positive. Let's rebase and get this moving, or close.

@luke-jr

This comment has been minimized.

Show comment Hide comment
@luke-jr

luke-jr Jul 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.

Member

luke-jr commented Jul 17, 2013

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

This comment has been minimized.

Show comment Hide comment
@jgarzik

jgarzik Aug 25, 2013

Contributor

Closing - non-responsive. Feel free to rebase and reopen.

Contributor

jgarzik commented Aug 25, 2013

Closing - non-responsive. Feel free to rebase and reopen.

@jgarzik jgarzik closed this Aug 25, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment