Skip to content

update AddressInfo struct #563

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

Merged

Conversation

Eunoia1729
Copy link
Contributor

@Eunoia1729 Eunoia1729 commented Mar 10, 2022

Description

Resolves #541.

  • Updates AddressInfo struct to include keychainKind
  • Updates the related get_address functions to pass in this field
  • Updates corresponding tests

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

tACK aa8c8f9

@notmandatory
Copy link
Member

Looks good! can you also add a new test to verify the KeychainKind changes for an internal address? A new function was added in the last release to let you do this:
https://docs.rs/bdk/latest/bdk/wallet/struct.Wallet.html#method.get_internal_address

Copy link

@ricknjacky ricknjacky left a comment

Choose a reason for hiding this comment

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

LGTM!

@Eunoia1729
Copy link
Contributor Author

Eunoia1729 commented Mar 25, 2022

Looks good! can you also add a new test to verify the KeychainKind changes for an internal address? A new function was added in the last release to let you do this: https://docs.rs/bdk/latest/bdk/wallet/struct.Wallet.html#method.get_internal_address

@notmandatory Thanks for the review !
Can I expand the existing tests instead of adding new ones ?
For example: I'm planning of expanding this test to assert the keyChainKind field as well along with address field. My reasoning behind this is to not increase the number of tests.

@rajarshimaitra
Copy link
Contributor

@Eunoia1729 I think the best thing to do here is modify the get_address test

fn test_get_address() {
and change all the asserts into full structure matching, instead of just the address strings..

@Eunoia1729 Eunoia1729 force-pushed the update-addressinfo-struct branch from aa8c8f9 to e277ac8 Compare March 25, 2022 07:32
@Eunoia1729
Copy link
Contributor Author

Eunoia1729 commented Mar 25, 2022

@Eunoia1729 I think the best thing to do here is modify the get_address test

Makes sense. Thanks for the inputs, @rajarshimaitra !
I've updated accordingly.

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

ReACK e277ac8

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK e277ac8

@notmandatory
Copy link
Member

Looks like Github glitched and didn't run the tests which prevents me from merging. @Eunoia1729 can you maybe try force pushing your branch again, or if that doesn't trigger CI you may need to amend you last commit message and then try force pushing that. Thanks!

@Eunoia1729 Eunoia1729 force-pushed the update-addressinfo-struct branch from e277ac8 to 2698fc0 Compare April 4, 2022 05:45
@Eunoia1729
Copy link
Contributor Author

can you maybe try force pushing your branch again

@notmandatory Sure, done !

@notmandatory notmandatory merged commit 9c0141b into bitcoindevkit:master Apr 5, 2022
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.

Add the KeychainKind to the AddressInfo struct
4 participants