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.

Sign up for GitHub

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

Jump to bottom

rpcclient: Add addressType to GenNewAddress #1567

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

rpcclient: Add addressType to GenNewAddress #1567

wants to merge 1 commit into from

Conversation

marsu-dev
Copy link

  • allow to create bech32 addresses

@marsu-dev
Add GetNewAddressCmd AddressType
5dc523e 
- allow to create bech32 addresses
@marsu-dev marsu-dev changed the title Add addressType to GenNewAddress rpcclient: Add addressType to GenNewAddress Apr 24, 2020
@marsu-dev marsu-dev force-pushed the rpcclient_wallet_addresstype branch from 42b1352 to 5dc523e Compare April 24, 2020 12:06
@torkelrogstad
Copy link
Contributor

Looks like this needs a rebase.

@marsu-dev
Copy link
Author

Sure, I have base my production code on tagged version of btcd
I see the conflict with the params for the client on the master.
If adding the optional argument is acceptable (and if no other methods are in the same case) I will rebase and test the changes

@torkelrogstad
Copy link
Contributor

I think is a nice change (note that I don't have merge permissions). One suggestion: add a btcjson.AddressType type that represents the different address types allowed (legacy, p2sh-segwit and bech32)

@onyb
Copy link
Contributor

onyb commented May 4, 2020

@michel-foucault For the btcjson.AddressType suggestion, here's an example of EstimateSmartFeeMode to help you.

@onyb onyb added the rpc label Sep 16, 2020
@onyb
Copy link
Contributor

onyb commented Sep 16, 2020

@michel-foucault Any update on this?

@marsu-dev
Copy link
Author

Hi, we used direct rpc call for our need.
I can update the branch and finalize it if you need

onyb
onyb suggested changes Sep 16, 2020
View reviewed changes
Copy link
Contributor

@onyb onyb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It would be great to have your changes in since they're mostly ready.

Summary of things to do for your reference:

  • Rebase and fix conflicts.
  • Use a special type for AddressType field, as suggested in a previous review.
  • Add a test for the case when account is unspecified, but address type is provided.

btcjson/walletsvrcmds_test.go
return btcjson.NewCmd("getnewaddress", "acct", "bech32")
},
staticCmd: func() interface{} {
return btcjson.NewGetNewAddressCmd(btcjson.String("acct"), btcjson.String("bech32"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add another test for the case when the label is nil and the address type is specified?

btcjson.NewGetNewAddressCmd(nil, btcjson.String("bech32"))

@jakesylvestre jakesylvestre mentioned this pull request Sep 25, 2020
Open
@jcvernaleo
Copy link
Member

I'd love to get this one in. Just needs those couple changes in the little checklist from @onyb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onyb onyb onyb requested changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
rpc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marsu-dev @torkelrogstad @onyb @jcvernaleo