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

Leverage constants from BitGoJS.statics #65

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wellkochi
Copy link

@wellkochi wellkochi commented Mar 15, 2020

Motivation

Use existing constants from BitGoJS.statics

Overview Of Changes

Use decimals from BitGoJS.statics in the following functions:

  • handleSignUtxo
  • signEthTx
  • handleSignXrp

Use networks from BitGoJS.statics in the following function:

  • handleSignUtxo

Use decimals from BitGoJS.statics in the following functions:
- handleSignUtxo 
- signEthTx 
- handleSignXrp 

Use networks from BitGoJS.statics in the following functions:
- handleSignUtxo
Copy link
Contributor

@marktoda marktoda left a comment

Choose a reason for hiding this comment

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

Looks good for the most part -- added some comments, and please add the issue # into your commit so others can look it up

app/sign.js Outdated Show resolved Hide resolved
app/sign.js Outdated Show resolved Hide resolved
app/sign.js Outdated

// const network = utxoNetworks[recoveryRequest.coin];
// const decimals = coinDecimals[recoveryRequest.coin];
const network = statics.coins.get[recoveryRequest.coin].network;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth adding an alias getCoinConfig(coin) to avoid having to write statics.coins.get[x] over and over

Copy link
Author

Choose a reason for hiding this comment

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

Good point, but consider it only used 4 times in this file so I didn't bother

Copy link
Author

@wellkochi wellkochi Mar 16, 2020

Choose a reason for hiding this comment

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

Added. Not sure if this is the optimal way tho.
1730b1b

Copy link
Author

@wellkochi wellkochi left a comment

Choose a reason for hiding this comment

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

#61

Using getCoinConfig(coin, data) instead of statics.coins.get[coin].data
BitGo#61
Copy link
Contributor

@marktoda marktoda left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

Copy link
Contributor

@marktoda marktoda left a comment

Choose a reason for hiding this comment

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

Actually, found some issues after looking it over again. Unfortunately our test coverage is not great in this repository. Could you add small tests to cover this change?

app/sign.js Outdated

const BCH_COINS = ['bch', 'tbch', 'bsv', 'tbsv'];
const TEN = new BN(10);

const EOS_MAINNET_CHAIN_ID = 'aca376f206b8fc25a6ed44dbdc66547c36c6c33e3a119ffbeaef943642f0e906';
const EOS_TESTNET_CHAIN_ID = 'e70aaab8997e1dfce58fbfac80cbbb8fecec7b99cf982a9444273cbc64c41473';

const getCoinConfig = function(coin, data) {
return statics.coins.get[coin].data;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think .data tries to get the key "data" out of the object, not the actual key passed as a param. Maybe you want to use [data]

Copy link
Author

Choose a reason for hiding this comment

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

Oh yeah. Didn't notice this is a reserved keyword

app/sign.js Outdated
const network = utxoNetworks[recoveryRequest.coin];
const decimals = coinDecimals[recoveryRequest.coin];

const network = getCoinConfig(recoveryRequest.coin, network);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are network and decimalPlaces defined here? Maybe you wanted to use string keys.

Copy link
Author

@wellkochi wellkochi Mar 18, 2020

Choose a reason for hiding this comment

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

Thanks for notice. I change the type var to string for network and decimalPlaces. Can you check if the lastest commit is legit? Haven't coded in JS for a long time so need to catch up its syntax.
cb650e8

Change var to string
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.

2 participants