Page MenuHomePhabricator

[token-server] Initialize wallet methods
ClosedPublic

Authored by bytesofman on Mar 29 2024, 04:48.

Details

Reviewers
emack
Group Reviewers
Restricted Project
Commits
rABC2574c313994d: [token-server] Initialize wallet methods
Summary

Initialize wallet methods that will be used to manage a hot wallet that sends token rewards to Cashtab users

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Branch
token-server-wallet
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 28147
Build 55839: Build Difftoken-server-tests
Build 55838: arc lint + arc unit

Event Timeline

remove unused dep, create constants dir (ref only for now), better todo

basic functions and unit tests for wallet generation

improve script, remove unrelated change

bytesofman added inline comments.
apps/token-server/constants/networks.ts
23 ↗(On Diff #46604)

This is not used yet and may be removed later. However, it is useful to keep it here until I work out how token-server will build transactions.

emack requested changes to this revision.Mar 30 2024, 06:57
emack added a subscriber: emack.
emack added inline comments.
apps/token-server/constants/networks.ts
7 ↗(On Diff #46607)

I can't find it anymore but wasn't this already in the monorepo somewhere? It was right around the time when we were deprecating bch-js and these tx builder constants were then plonked into a standalone file. Was it deprecated or am I not looking in the right places?

apps/token-server/scripts/getWallet.ts
41 ↗(On Diff #46607)

I'm screencapping this next time Tobias throws shade at JS

43 ↗(On Diff #46607)

on a semi-related note, since we've got half an eye on supporting HD wallets in the near future, is it worth keeping half an eye on supporting 24 word seed wallets as well? If so, might be worth having an optional param that defaults to 12 word, but the callsite can optionally override with the entropy bits for 24 words?

57 ↗(On Diff #46607)
apps/token-server/src/wallet.ts
31 ↗(On Diff #46607)

Are you intending to support p2sh addresses here as well? If not, needs a filter here.

41 ↗(On Diff #46607)

what's the need for the 1 here? Isn't the derivation path fixed to 1899 above so you'll only ever have one child here?

apps/token-server/test/vectors.ts
688 ↗(On Diff #46607)

As noted in getWalletFromSeed() above, add a test to check how it behaves with a valid 12-word bip39 seed for a p2sh address

This revision now requires changes to proceed.Mar 30 2024, 06:57
bytesofman marked 7 inline comments as done.

child1 is a dumb name, improve getWallet script to handle async function

apps/token-server/constants/networks.ts
7 ↗(On Diff #46607)

we had these in cashtab but I removed the file as it was no longer used

apps/token-server/scripts/getWallet.ts
43 ↗(On Diff #46607)

since this function is only for the server's hot wallet, we do not need it to be extensible or flexible.

the security vector of "private key will be on a server" is many millions of times higher than "mnemonic is 12 words instead of 24 words."

imo same for cashtab. risk of forgetting a 24-word seed is much greater than risk of 12 word seed being hacked by some kind of tool that would fail to hack a 24-word seed.

57 ↗(On Diff #46607)

we cannot use await outside the scope of a function. that said, still possible to improve how this is handled. updated.

apps/token-server/src/wallet.ts
31 ↗(On Diff #46607)

nah at the moment this is all just for the hot wallet on this server which will send token rewards to cashtab users. we don't want support for anything else.

41 ↗(On Diff #46607)

good point. copypasta from bitcoinjs-lib tests I was modeling.

apps/token-server/test/vectors.ts
688 ↗(On Diff #46607)

going to avoid this complexity as we do not need to support it.

This revision is now accepted and ready to land.Mar 30 2024, 14:32
This revision was automatically updated to reflect the committed changes.