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

Feature/wallet preparetx #24

Merged
merged 28 commits into from
Nov 5, 2018
Merged

Feature/wallet preparetx #24

merged 28 commits into from
Nov 5, 2018

Conversation

akekeke
Copy link
Contributor

@akekeke akekeke commented Oct 24, 2018

WIP

  • need to write testcases

This closes #6

@Jwata
Copy link
Contributor

Jwata commented Oct 29, 2018

@akekeke
I'm considering restructuring the logic of preparing fund tx from wallet to dlc, so wallet can focus only on preparing txins(and a out if there is charge) from utxos.

@akekeke akekeke force-pushed the feature/wallet-preparetx branch from 5b12cef to 4416e31 Compare October 30, 2018 10:21
@akekeke
Copy link
Contributor Author

akekeke commented Nov 1, 2018

Sorry this PR is so large ><; ListUnspent() is longer than I thought it would be.

This PR:

  • refactored Wallet creation. DB and Wallet creation separated into 2 functions.
  • implemented ListUnspent(). Its complexity comes from converting Credit type into btcjson.ListUnspentResult type :,(

No test is written for ListUnspent() yet

internal/wallet/utxo.go Outdated Show resolved Hide resolved
@akekeke akekeke force-pushed the feature/wallet-preparetx branch from aa22de4 to a560051 Compare November 2, 2018 07:04
internal/wallet/address.go Outdated Show resolved Hide resolved
@@ -111,7 +112,7 @@ func Create(db walletdb.DB, params *chaincfg.Params, seed, pubPass,

// Open loads a wallet from the passed db and public pass phrase.
func Open(db walletdb.DB, pubPass []byte,
params *chaincfg.Params) (Wallet, error) {
params *chaincfg.Params) (*wallet, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this function should return a struct instead of an interface, because the wallet struct already fulfills the Wallet interface.

What do you think? @Jwata

Copy link
Contributor

Choose a reason for hiding this comment

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

As golint tells us, caller doesn't know about wallet because it's private.

exported func Open returns unexported type *wallet.wallet, which can be annoying to use

Related to this discussion.
golang/lint#210

@akekeke
Copy link
Contributor Author

akekeke commented Nov 2, 2018

This PR:

  • added privatePassphrase as a parameter in the wallet struct
  • refactored CreateWallet() and Open() to return wallet struct instead of interface
  • implemented wallet.ListUnspent()

internal/wallet/utxo_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Jwata Jwata left a comment

Choose a reason for hiding this comment

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

I will review more closely later, but leave some comments.
It seems better to work together to finish wallet task as it's getting complicated to manage managers...

@@ -111,7 +112,7 @@ func Create(db walletdb.DB, params *chaincfg.Params, seed, pubPass,

// Open loads a wallet from the passed db and public pass phrase.
func Open(db walletdb.DB, pubPass []byte,
params *chaincfg.Params) (Wallet, error) {
params *chaincfg.Params) (*wallet, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As golint tells us, caller doesn't know about wallet because it's private.

exported func Open returns unexported type *wallet.wallet, which can be annoying to use

Related to this discussion.
golang/lint#210

"github.com/btcsuite/btcutil"
"github.com/btcsuite/btcwallet/waddrmgr"
"github.com/btcsuite/btcwallet/walletdb"
)

func (w *wallet) NewPubkey() (pub *btcec.PublicKey, err error) {
// TODO: generate new pubkey and address using newAddress
_, err = w.newAddress(waddrmgr.KeyScopeBIP0084, []byte{}, uint32(1), uint32(1))
mAddrs, err := w.newAddress(waddrmgr.KeyScopeBIP0084, w.privatePassphrase, uint32(1), uint32(1))
Copy link
Contributor

Choose a reason for hiding this comment

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

I still wonder why you need private key, though.
I guess the manager already is unlocked? so you don't need to unlock it everytime you write.
https://github.com/btcsuite/btcwallet/blob/c4dd27e481f9801866cf5226bfe532084772ec2a/wallet/wallet.go#L2830

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that a private kay was necessary for creating managers, but not for modifying records.

internal/wallet/test_util.go Outdated Show resolved Hide resolved
addrmgrNs := tx.ReadBucket(waddrmgrNamespaceKey)
txmgrNs := tx.ReadBucket(wtxmgrNamespaceKey)

syncBlock := w.manager.SyncedTo()
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the waddmgr do some block downloading task?
I though it just manages addresses and priv/pub keys of them.

@@ -48,14 +48,14 @@ func setupDB(t *testing.T) (db walletdb.DB, tearDownFunc func()) {
return
}

func setupWallet(t *testing.T) (tearDownFunc func(), w Wallet) {
func setupWallet(t *testing.T) (tearDownFunc func(), w Wallet, db walletdb.DB) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some tests need to access the wallet's db directly, so that's why I added db as one of returned values


Manager() *waddrmgr.Manager
TxStore() *wtxmgr.Store
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added getter functions so functions that use the Wallet interface have access to wallet's manager and txStore

@akekeke
Copy link
Contributor Author

akekeke commented Nov 5, 2018

This PR:

  • CreateWallet returns a Wallet interface instead of wallet struct (changed from an earlier PR)
  • added getter functions for wallet's manager and txStore

@Jwata Jwata force-pushed the feature/wallet-preparetx branch from 72f1c30 to 8b77ef5 Compare November 5, 2018 04:48
Copy link
Contributor

@Jwata Jwata left a comment

Choose a reason for hiding this comment

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

LGTM. Let's finish in next PR.

@akekeke akekeke merged commit 22fb197 into master Nov 5, 2018
@akekeke akekeke deleted the feature/wallet-preparetx branch November 16, 2018 06:50
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.

Prepare tx inputs/outputs for fund tx
2 participants