[WIP] Add testnet support #803
Conversation
| NETWORK_ETHEREUM: 1, | ||
| NETWORK_BCH: BitcoinCash.networks.bitcoin | ||
| NETWORK_BCH: BitcoinCash.networks[isTestnet ? 'testnet' : 'bitcoin'] |
There was a problem hiding this comment.
Small nitpick. We're trying to use currency codes instead of currency names. Can you change to NETWORK_BTC?
There was a problem hiding this comment.
If you hardcode testnet for now, I noticed a failure and helpful :) note here: http://github.com/blockchain/blockchain-wallet-v4-frontend/blob/e6b67ab820bb6e262dfb2f89ac3f3389d1c1fcb6/packages/blockchain-wallet-v4/src/types/HDAccount.js#L93
There was a problem hiding this comment.
I also noticed we are hardcoding Bitcoin.networks.bitcoin in a few places, which will need to be replaced by settings config. For example: http://github.com/blockchain/blockchain-wallet-v4-frontend/blob/e6b67ab820bb6e262dfb2f89ac3f3389d1c1fcb6/packages/blockchain-wallet-v4/src/types/HDWallet.js#L88
There was a problem hiding this comment.
@plondon Thank you!
- I will address the naming.
- Good catch! I caught that TODO as well and has addressed it in local.
- Great! I will search for these hardcoding reference
| @@ -1,13 +1,15 @@ | |||
| import Bitcoin from 'bitcoinjs-lib' | |||
| import BitcoinCash from 'bitcoinforksjs-lib' | |||
|
|
|||
| const isTestnet = process.NODE_ENV === 'testnet' | |||
There was a problem hiding this comment.
Correct is process.env.NODE_ENV, also this doesn't seem to work even if it's correct
There was a problem hiding this comment.
Apologize for my late night stupidness. This will not work because process.env.NODE_ENV will not be known by the client. I will address it.
lzhuor
force-pushed
the
fix/testnet_support
branch
2 times, most recently
from
6259340
to
e85f97e
Compare
- Fixed config/index.js by renaming 'NETWORK' to 'NETWORK_BITCOIN' - Setting network config conditionally by checking environment - Passing network from onSubmit function of Register component - Fixed a few linting issues
- Rebased development branch
- Rebased development branch
lzhuor
force-pushed
the
fix/testnet_support
branch
from
e85f97e
to
3129139
Compare
| @@ -1,13 +1,15 @@ | |||
| import Bitcoin from 'bitcoinjs-lib' | |||
| import BitcoinCash from 'bitcoinforksjs-lib' | |||
|
|
|||
| const isTestnet = false | |||
There was a problem hiding this comment.
I think a better way to do this is to determine the network through wallet-options. https://github.com/blockchain/wallet-options/blob/master/prod/wallet-options-v4.json#L30. So I will probably end up removing NETWORK related things from this config
…emove btc reference in settings config
- Add fallbackFees when mempool/fees fails
|
@lzhuor thanks for starting this off. I put a bit more work in but there are still a few todos before we can merge it. Sending works, but only if you override the isValidAddress check here: https://github.com/lzhuor/blockchain-wallet-v4-frontend/blob/fix/testnet_support/packages/blockchain-wallet-v4/src/utils/btc.js#L16. We need to pass the network to this function. |
|
@plondon Thank you for your patience! I was drenched in my personal stuff in the past week. Looking into it now! |
|
@lzhuor apology unnecessary, really appreciate your contribution! |
|
Not sure why the build failed but moving this to another PR |
WORK IN PROGRESS, PLEASE DO NOT MERGE.
TODO
Description
Added logic to check Node.js environment in
config/index.jsso Bitcoin Network and Bitcoin Cash Network can be derived fromNODE_ENVChange Type
Feature
Added testnet support
Bug Fix
networkis assigned withsettings.NETWORK_BITCOINbut we are usingNETWORKas the name inconfig/index.jsso its value is alwaysundefined. RenamedNETWORKasNETWORK_BITCOINnetworkas a param inactions.auth.register()functionTesting Steps
yarn start:testnetCode Checklist
yarn start)yarn lint)yarn test)README.mdand other documentation is updated as needed