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

wallet: support alternative signers #145

Merged
merged 6 commits into from
Jun 11, 2019
Merged

wallet: support alternative signers #145

merged 6 commits into from
Jun 11, 2019

Conversation

boymanjor
Copy link
Contributor

@boymanjor boymanjor commented Apr 11, 2019

This PR introduces new RPC calls which create unsigned txs for each covenant type. It also adds a new WalletCoinView class that extends CoinView and stores HD paths. These paths allow alternative, HD wallets to create valid input signatures. The MTX class has also been updated to persist coin information during de/serialization. Finally, this commit adds support for unsigned auction transactions to the HTTP API.

End-to-end tests can be found in https://github.com/boymanjor/hsd-ledger and require a Ledger Nanos S hardware wallet.

@codecov-io
Copy link

codecov-io commented Apr 11, 2019

Codecov Report

Merging #145 into master will increase coverage by 2.91%.
The diff coverage is 68.8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
+ Coverage   50.05%   52.96%   +2.91%     
==========================================
  Files         126      129       +3     
  Lines       35403    35746     +343     
  Branches     5998     6026      +28     
==========================================
+ Hits        17722    18934    +1212     
+ Misses      17681    16812     -869
Impacted Files Coverage Δ
lib/wallet/path.js 76.05% <100%> (+5.11%) ⬆️
lib/wallet/http.js 65.03% <100%> (+1.12%) ⬆️
lib/primitives/mtx.js 68.38% <100%> (+0.35%) ⬆️
lib/primitives/input.js 87.5% <100%> (+0.12%) ⬆️
lib/primitives/tx.js 72.96% <100%> (+0.03%) ⬆️
lib/coins/coinview.js 75.54% <50%> (+8.32%) ⬆️
lib/wallet/paths.js 52.38% <52.38%> (ø)
lib/wallet/walletcoinview.js 62.06% <62.06%> (ø)
lib/wallet/rpc.js 21.93% <64%> (+10.96%) ⬆️
lib/wallet/wallet.js 61.41% <80.76%> (+2.68%) ⬆️
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 189f54d...a451b90. Read the comment docs.

@boymanjor boymanjor force-pushed the create-rpcs branch 3 times, most recently from f4b6216 to b85c2cc Compare April 22, 2019 23:01
@boymanjor boymanjor changed the title WIP: allow creation of unsigned auction txs wallet: allow creation of unsigned auction txs Apr 22, 2019
@boymanjor boymanjor marked this pull request as ready for review April 22, 2019 23:03
@boymanjor boymanjor changed the title wallet: allow creation of unsigned auction txs wallet: support alternative signers Apr 22, 2019
lib/wallet/wallet.js Outdated Show resolved Hide resolved
@tynes
Copy link
Contributor

tynes commented Apr 23, 2019

What do you think about porting over the end to end tests? That would make this PR way larger and would cause a big rebase for #115, but we probably want to move them over here eventually

@boymanjor
Copy link
Contributor Author

What do you think about porting over the end to end tests? That would make this PR way larger and would cause a big rebase for #115, but we probably want to move them over here eventually

I think you are right. I'll update the end-to-end tests from hsd-ledger to sign using another hsd wallet and merge them here.

@boymanjor
Copy link
Contributor Author

What do you think about porting over the end to end tests? That would make this PR way larger and would cause a big rebase for #115, but we probably want to move them over here eventually

I think you are right. I'll update the end-to-end tests from hsd-ledger to sign using another hsd wallet and merge them here.

On second thought. It is probably a better idea to add sufficient unit and integration tests here.

@tynes
Copy link
Contributor

tynes commented Apr 24, 2019

What do you think about passing along paths here now that the function signature has been updated for input.getJSON? This is in TX.getJSON

hsd/lib/primitives/tx.js

Lines 1749 to 1752 in af86d48

inputs: this.inputs.map((input) => {
const coin = view ? view.getCoinFor(input) : null;
return input.getJSON(network, coin);
}),

@tynes
Copy link
Contributor

tynes commented May 6, 2019

@boymanjor Let me know if there is anything that I can do to help get this over the finishline

lib/primitives/mtx.js Outdated Show resolved Hide resolved
lib/primitives/mtx.js Outdated Show resolved Hide resolved
@nodech
Copy link
Contributor

nodech commented May 8, 2019

For multisig wallets that need redeem scripts to actually sign the transactions, it might be good idea to also provide redeem script within CoinView.

Note that it's still possible to construct redeem scripts with this CoinView (without introducing redeem scripts in it), get wallet keys from account, and for each mapping of hash -> derivation, derive public keys for each of the accountKey + keys, resulting in redeem script.

@boymanjor
Copy link
Contributor Author

@tynes this is ready for another review.

test/path-test.js Outdated Show resolved Hide resolved
test/wallet-rpc-test.js Outdated Show resolved Hide resolved
test/wallet-rpc-test.js Outdated Show resolved Hide resolved
test/wallet-rpc-test.js Outdated Show resolved Hide resolved
test/wallet-rpc-test.js Outdated Show resolved Hide resolved
test/wallet-rpc-test.js Outdated Show resolved Hide resolved
test/path-test.js Outdated Show resolved Hide resolved
test/path-test.js Show resolved Hide resolved
test/data/mtx1.json Outdated Show resolved Hide resolved
lib/primitives/input.js Show resolved Hide resolved
@tynes
Copy link
Contributor

tynes commented Jun 8, 2019

The Details class in lib/wallet/txdb.js may be relevant to this PR

https://github.com/handshake-org/hsd/blob/master/lib/wallet/txdb.js#L3339

@boymanjor boymanjor force-pushed the create-rpcs branch 2 times, most recently from 54c6063 to 5001478 Compare June 11, 2019 17:42
This commit includes a new class WalletCoinView and a helper
class Paths. WalletCoinView inherits from CoinView and adds
the ability to store HD paths on coin viewpoint objects.
This commit exposes a new 'network' argument to `toPath`. Passing
the network type will generate a full HD path instead of the current,
abbreviated path.
test/auction-rpc-test.js Outdated Show resolved Hide resolved
@tynes
Copy link
Contributor

tynes commented Jun 11, 2019

Looks good to me

@boymanjor boymanjor merged commit e1c0281 into master Jun 11, 2019
@boymanjor boymanjor deleted the create-rpcs branch August 26, 2019 19:00
prefix += `/${purpose}'/${network.keyPrefix.coinType}'`;
}

return `${prefix}/${this.account}'/${this.branch}/${this.index}`;
Copy link
Contributor

@nodech nodech Sep 11, 2019

Choose a reason for hiding this comment

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

This might be an issue whenever this.account != this.accountKey.childIndex..
Related to: bcoin-org/bcoin#696

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.

4 participants