Skip to content

Wallet derive address#3382

Closed
rebrahimi1 wants to merge 3 commits into
bitpay:masterfrom
rebrahimi1:walletDeriveAddress
Closed

Wallet derive address#3382
rebrahimi1 wants to merge 3 commits into
bitpay:masterfrom
rebrahimi1:walletDeriveAddress

Conversation

@rebrahimi1

Copy link
Copy Markdown
Contributor

deriving addresses with change option

const main = async () => {
const { name, path, storageType, addressIndexes } = program;
let { change } = program;
change = (change === 'true');

@kylexrose kylexrose Jul 31, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this is a bit verbose, did you ever think about destructuring this in line 19? i.e.
const { name, path, storageType, addressIndexes , change=(change === 'true')} = program;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it looks like destructuring it will not result change to boolean true/false, which has it be boolean. Another way that I can think of is to check if change is defined to avoid the program to crash and do JSON.parse(change).

This comment was marked as spam.

await wallet.register({ baseUrl: wallet.baseUrl });

if (addressIndexes) {
indexes = JSON.parse(addressIndexes).indexes;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

since you have dot notation here, it seems you expect addressIndexes to be an object though I'm not sure it is.


for (let i = 0; i < indexes.length; i++) {
address = wallet.deriveAddress(indexes[i], change);
if (address) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If you're setting the address right before this than would you expect address to be defined? -- and if there is a problem deriving one address, is it skipping to line 61?

console.log('Addresses & Indexes: ', addressesAndIndex);
}
} catch (e) {
console.error(e);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe a clearer error to the user here/another try block somewhere since there is more code than in the original util.

.version(require('../package.json').version)
.option('--name <name>', 'REQUIRED - Wallet Name')
.option('--change [change]', 'optional - should derive change addresses')
.option('--addressIndexes [addressIndexes]', 'optional - takes one or more indexes')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I believe the request was for a range (i.e. 4-20) vs an array (i.e. [4, 34, 40, 6]) I'm not sure the use case for deriving from an array of indexes but it's just a thought.

@escottalexander

Copy link
Copy Markdown
Contributor

This has been replaced by #3460. It can be closed.

@kajoseph

Copy link
Copy Markdown
Collaborator

replaced by #3460

@kajoseph kajoseph closed this Nov 10, 2022
@Paulalobe

Paulalobe commented Nov 11, 2022 via email

Copy link
Copy Markdown

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.

5 participants