Add API for internal addresses - #522

Merged
notmandatory merged 2 commits into
bitcoindevkit:masterfrom
LLFourn:get_change_address
Mar 2, 2022
Merged

Add API for internal addresses#522
notmandatory merged 2 commits into
bitcoindevkit:masterfrom
LLFourn:get_change_address

Conversation

@LLFourn

@LLFournLLFourn commented Jan 11, 2022

Copy link
Copy Markdown
Collaborator

There are good reasons for applications to need to get internal
addresses too. For example creating a transactions that splits an output
into several smaller ones.

Checklists

All Submissions:

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@LLFournLLFourn mentioned this pull request Jan 11, 2022
Closed
@LLFourn
LLFournforce-pushed the get_change_address branch 2 times, most recently from 1546c42 to 63f1ec0CompareJanuary 11, 2022 01:29
rajarshimaitra
rajarshimaitra reviewed Jan 11, 2022
View reviewed changes

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Concept + tACK 63f1ec0

I think it makes sense and the code looks good to me.

w0xlt
w0xlt approved these changes Jan 11, 2022
View reviewed changes

@w0xltw0xlt left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tACK 63f1ec0

I tested this commit with two wallets:

1 - with internal and external descriptors: worked as expected.

2- with external descriptor only: get_internal_address returns the same addresses as get_address.

Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .

But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,

rajarshimaitra
rajarshimaitra reviewed Jan 12, 2022
View reviewed changes
Comment threadsrc/wallet/mod.rs Outdated
@rajarshimaitra

rajarshimaitra commented Jan 12, 2022

Copy link
Copy Markdown
Contributor

Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .
But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,

I think the get_descriptor_for_keychain() is written that way to make wallet api agnostic of change descriptor. Weather its there or not, all the logic will simply follow "use external if there's no internal". This simplifies internal logic handling at many places and doesn't force us to handle "no change desc case" separately. and the lib user just keep this in mind while using the wallet. Its also documented in many places.

So it makes sense to me to keep the behavior intact in peek_address too.

@w0xltw0xlt mentioned this pull request Jan 13, 2022
Closed
rajarshimaitra
rajarshimaitra previously requested changes Jan 14, 2022
View reviewed changes

@rajarshimaitrarajarshimaitra left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ACK 89f88ce

Sorry I missed few others comment nits in last review, of the previous methods.

Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
@LLFourn

LLFourn commented Jan 25, 2022

Copy link
Copy Markdown
CollaboratorAuthor

comments addressed ready for further review.

@notmandatory

notmandatory commented Feb 2, 2022•
edited
Loading

Copy link
Copy Markdown
Member

I think docs are breaking because of a regression in rust +nightly, there seems to be a bug fix PR in the works so I think we should wait a couple days and see if that fixes it. See #538

@notmandatory

notmandatory commented Feb 11, 2022

Copy link
Copy Markdown
Member

#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.

notmandatory
notmandatory requested changes Feb 11, 2022
View reviewed changes

@notmandatorynotmandatory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, one small thing to fix in the new test.

Comment threadsrc/wallet/mod.rs Outdated
.unwrap();

assert_eq!(
wallet.get_address(AddressIndex::New).unwrap().address,

@notmandatorynotmandatoryFeb 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't you be verifying that the internal address is the same as the external here?

Suggested change
wallet.get_address(AddressIndex::New).unwrap().address,
wallet.get_internal_address(AddressIndex::New).unwrap().address,

@LLFournLLFournFeb 15, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

thanks fixed @notmandatory

@notmandatory

notmandatory commented Feb 15, 2022

Copy link
Copy Markdown
Member

#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.

I think you still need to rebase since this went in, that will fix the rust docs CI job.

@nickfarrownickfarrow mentioned this pull request Feb 16, 2022
Closed
6 tasks
@LLFourn

LLFourn commented Feb 16, 2022•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

@notmandatory new CI problem. Looks like tokio increased MSRV without major version bump. I think we should set MSRV for bdk to v1.56 (2021 edition) and leave it there.

@notmandatory

notmandatory commented Feb 17, 2022

Copy link
Copy Markdown
Member

I guess we need to revisit and not close #331 .. at this point I agree the best option is set MSRV to rust 1.56, it's not the current stable for Debian.. but is being tested and should be shipped with next stable debian "bookworm" release. https://tracker.debian.org/pkg/rustc

@notmandatorynotmandatory mentioned this pull request Feb 17, 2022
Closed
3 tasks
LLFournand others added 2 commits February 23, 2022 09:28
@LLFourn
Add API for internal addresses
00f0901
There are good reasons for applications to need to get internal
addresses too. For example creating a transactions that splits an output
into several smaller ones.
@LLFourn@rajarshimaitra
Fix comment on peek_address
022256c
Co-authored-by: Raj <36541669+rajarshimaitra@users.noreply.github.com>
@LLFourn

LLFourn commented Feb 22, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Rebased. Should be ready for merge after CI (hopefully) passes.

notmandatory
notmandatory approved these changes Mar 2, 2022
View reviewed changes

@notmandatorynotmandatory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK 022256c

@notmandatory
notmandatory dismissed rajarshimaitra’s stale reviewMarch 2, 2022 04:48

Requested changes look to be resolved.

@bitcoindevkitbitcoindevkit deleted a comment from codecovBotMar 2, 2022
@notmandatory
notmandatory merged commit f0188f4 into bitcoindevkit:masterMar 2, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@notmandatorynotmandatorynotmandatory approved these changes
+2 more reviewers
@w0xltw0xltw0xlt approved these changes
@rajarshimaitrarajarshimaitrarajarshimaitra left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@LLFournLLFourn

Labels

None yet

Projects

None yet

Milestone

Release 0.17.0 Feature Freeze

Development

Successfully merging this pull request may close these issues.

4 participants

@LLFourn@rajarshimaitra@notmandatory@w0xlt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Add API for internal addresses - #522
Merged
notmandatory merged 2 commits into
bitcoindevkit:masterfrom 
LLFourn:get_change_address
Mar 2, 2022
Merged
Add API for internal addresses#522
notmandatory merged 2 commits into
bitcoindevkit:masterfrom 
LLFourn:get_change_address
Conversation
@LLFourn
@LLFournLLFourn
commented
Jan 11, 2022

Copy link


Copy Markdown

Collaborator
There are good reasons for applications to need to get internal

addresses too. For example creating a transactions that splits an output

into several smaller ones.
Checklists
All Submissions:
  •  I've signed all my commits
  •  I followed the contribution guidelines
  •  I ran cargo fmt and cargo clippy before committing
New Features:
  •  I've added tests for the new feature
  •  I've added docs for the new feature
  •  I've updated CHANGELOG.md
@LLFournLLFourn
mentioned this pull request
Jan 11, 2022
 Closed

@LLFourn
LLFournforce-pushed
the
get_change_address
branch
2 times, most recently
from
1546c42 to
63f1ec0CompareJanuary 11, 2022 01:29
rajarshimaitra
rajarshimaitra
reviewed
Jan 11, 2022
View reviewed changes
@rajarshimaitrarajarshimaitra
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Concept + tACK 63f1ec0
I think it makes sense and the code looks good to me.
w0xlt
w0xlt
approved these changes
Jan 11, 2022
View reviewed changes
@w0xltw0xlt
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

tACK 63f1ec0
I tested this commit with two wallets:
1 - with internal and external descriptors: worked as expected.
2- with external descriptor only: get_internal_address returns the same addresses as get_address.
Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .
But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,
rajarshimaitra
rajarshimaitra
reviewed
Jan 12, 2022
View reviewed changes
Comment threadsrc/wallet/mod.rs
Outdated

@rajarshimaitra
rajarshimaitra
commented
Jan 12, 2022

Copy link


Copy Markdown

Contributor
Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .

But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,
I think the get_descriptor_for_keychain() is written that way to make wallet api agnostic of change descriptor. Weather its there or not, all the logic will simply follow "use external if there's no internal". This simplifies internal logic handling at many places and doesn't force us to handle "no change desc case" separately. and the lib user just keep this in mind while using the wallet. Its also documented in many places.
So it makes sense to me to keep the behavior intact in peek_address too.
@w0xltw0xlt
mentioned this pull request
Jan 13, 2022
 Closed

rajarshimaitra
rajarshimaitra
previously requested changes
Jan 14, 2022
View reviewed changes
@rajarshimaitrarajarshimaitra
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

ACK 89f88ce
Sorry I missed few others comment nits in last review, of the previous methods.
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
@LLFourn
LLFourn
commented
Jan 25, 2022

Copy link


Copy Markdown

CollaboratorAuthor
comments addressed ready for further review.
@notmandatory
notmandatory
commented
Feb 2, 2022•

edited

Loading

Copy link


Copy Markdown

Member
I think docs are breaking because of a regression in rust +nightly, there seems to be a bug fix PR in the works so I think we should wait a couple days and see if that fixes it. See #538
@notmandatory
notmandatory
commented
Feb 11, 2022

Copy link


Copy Markdown

Member
#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.
notmandatory
notmandatory
requested changes
Feb 11, 2022
View reviewed changes
@notmandatorynotmandatory
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good, one small thing to fix in the new test.
Comment threadsrc/wallet/mod.rs
Outdated

 .unwrap();

 assert_eq!(
 wallet.get_address(AddressIndex::New).unwrap().address,
@notmandatorynotmandatoryFeb 11, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Shouldn't you be verifying that the internal address is the same as the external here?

Suggested change

 wallet.get_address(AddressIndex::New).unwrap().address,
 wallet.get_internal_address(AddressIndex::New).unwrap().address,
@LLFournLLFournFeb 15, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

thanks fixed @notmandatory
@notmandatory
notmandatory
commented
Feb 15, 2022

Copy link


Copy Markdown

Member
#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.
I think you still need to rebase since this went in, that will fix the rust docs CI job.
@nickfarrownickfarrow
mentioned this pull request
Feb 16, 2022
 Closed

6 tasks
@LLFourn
LLFourn
commented
Feb 16, 2022•

edited

Loading

Copy link


Copy Markdown

CollaboratorAuthor
@notmandatory new CI problem. Looks like tokio increased MSRV without major version bump. I think we should set MSRV for bdk to v1.56 (2021 edition) and leave it there.
@notmandatory
notmandatory
commented
Feb 17, 2022

Copy link


Copy Markdown

Member
I guess we need to revisit and not close #331 .. at this point I agree the best option is set MSRV to rust 1.56, it's not the current stable for Debian.. but is being tested and should be shipped with next stable debian "bookworm" release. https://tracker.debian.org/pkg/rustc
@notmandatorynotmandatory
mentioned this pull request
Feb 17, 2022
 Closed

3 tasks
LLFournand others
added 2 commits
February 23, 2022 09:28
@LLFourn
Add API for internal addresses
00f0901
There are good reasons for applications to need to get internal
addresses too. For example creating a transactions that splits an output
into several smaller ones.
@LLFourn@rajarshimaitra
Fix comment on peek_address
022256c
Co-authored-by: Raj <36541669+rajarshimaitra@users.noreply.github.com>
@LLFourn
LLFourn
commented
Feb 22, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased. Should be ready for merge after CI (hopefully) passes.
notmandatory
notmandatory
approved these changes
Mar 2, 2022
View reviewed changes
@notmandatorynotmandatory
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

ACK 022256c
@notmandatory
notmandatory
dismissed
rajarshimaitra’s stale reviewMarch 2, 2022 04:48
Requested changes look to be resolved.
@bitcoindevkitbitcoindevkit
deleted a comment from codecovBotMar 2, 2022
@notmandatory
notmandatory
merged commit f0188f4
into
bitcoindevkit:masterMar 2, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@notmandatorynotmandatorynotmandatory approved these changes

+2 more reviewers

@w0xltw0xltw0xlt approved these changes
@rajarshimaitrarajarshimaitrarajarshimaitra left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@LLFournLLFourn

Labels


None yet


Projects


None yet


Milestone

Release 0.17.0 Feature Freeze

Development

Successfully merging this pull request may close these issues.

4 participants

@LLFourn@rajarshimaitra@notmandatory@w0xlt

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add API for internal addresses - #522
Merged
notmandatory merged 2 commits into
bitcoindevkit:masterfrom 
LLFourn:get_change_address
Mar 2, 2022
Merged
Add API for internal addresses#522
notmandatory merged 2 commits into
bitcoindevkit:masterfrom 
LLFourn:get_change_address
Conversation
@LLFourn
@LLFournLLFourn
commented
Jan 11, 2022

Copy link


Copy Markdown

Collaborator
There are good reasons for applications to need to get internal

addresses too. For example creating a transactions that splits an output

into several smaller ones.
Checklists
All Submissions:
  •  I've signed all my commits
  •  I followed the contribution guidelines
  •  I ran cargo fmt and cargo clippy before committing
New Features:
  •  I've added tests for the new feature
  •  I've added docs for the new feature
  •  I've updated CHANGELOG.md
@LLFournLLFourn
mentioned this pull request
Jan 11, 2022
 Closed

@LLFourn
LLFournforce-pushed
the
get_change_address
branch
2 times, most recently
from
1546c42 to
63f1ec0CompareJanuary 11, 2022 01:29
rajarshimaitra
rajarshimaitra
reviewed
Jan 11, 2022
View reviewed changes
@rajarshimaitrarajarshimaitra
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Concept + tACK 63f1ec0
I think it makes sense and the code looks good to me.
w0xlt
w0xlt
approved these changes
Jan 11, 2022
View reviewed changes
@w0xltw0xlt
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

tACK 63f1ec0
I tested this commit with two wallets:
1 - with internal and external descriptors: worked as expected.
2- with external descriptor only: get_internal_address returns the same addresses as get_address.
Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .
But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,
rajarshimaitra
rajarshimaitra
reviewed
Jan 12, 2022
View reviewed changes
Comment threadsrc/wallet/mod.rs
Outdated

@rajarshimaitra
rajarshimaitra
commented
Jan 12, 2022

Copy link


Copy Markdown

Contributor
Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .

But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,
I think the get_descriptor_for_keychain() is written that way to make wallet api agnostic of change descriptor. Weather its there or not, all the logic will simply follow "use external if there's no internal". This simplifies internal logic handling at many places and doesn't force us to handle "no change desc case" separately. and the lib user just keep this in mind while using the wallet. Its also documented in many places.
So it makes sense to me to keep the behavior intact in peek_address too.
@w0xltw0xlt
mentioned this pull request
Jan 13, 2022
 Closed

rajarshimaitra
rajarshimaitra
previously requested changes
Jan 14, 2022
View reviewed changes
@rajarshimaitrarajarshimaitra
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

ACK 89f88ce
Sorry I missed few others comment nits in last review, of the previous methods.
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
@LLFourn
LLFourn
commented
Jan 25, 2022

Copy link


Copy Markdown

CollaboratorAuthor
comments addressed ready for further review.
@notmandatory
notmandatory
commented
Feb 2, 2022•

edited

Loading

Copy link


Copy Markdown

Member
I think docs are breaking because of a regression in rust +nightly, there seems to be a bug fix PR in the works so I think we should wait a couple days and see if that fixes it. See #538
@notmandatory
notmandatory
commented
Feb 11, 2022

Copy link


Copy Markdown

Member
#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.
notmandatory
notmandatory
requested changes
Feb 11, 2022
View reviewed changes
@notmandatorynotmandatory
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good, one small thing to fix in the new test.
Comment threadsrc/wallet/mod.rs
Outdated

 .unwrap();

 assert_eq!(
 wallet.get_address(AddressIndex::New).unwrap().address,
@notmandatorynotmandatoryFeb 11, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Shouldn't you be verifying that the internal address is the same as the external here?

Suggested change

 wallet.get_address(AddressIndex::New).unwrap().address,
 wallet.get_internal_address(AddressIndex::New).unwrap().address,
@LLFournLLFournFeb 15, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

thanks fixed @notmandatory
@notmandatory
notmandatory
commented
Feb 15, 2022

Copy link


Copy Markdown

Member
#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.
I think you still need to rebase since this went in, that will fix the rust docs CI job.
@nickfarrownickfarrow
mentioned this pull request
Feb 16, 2022
 Closed

6 tasks
@LLFourn
LLFourn
commented
Feb 16, 2022•

edited

Loading

Copy link


Copy Markdown

CollaboratorAuthor
@notmandatory new CI problem. Looks like tokio increased MSRV without major version bump. I think we should set MSRV for bdk to v1.56 (2021 edition) and leave it there.
@notmandatory
notmandatory
commented
Feb 17, 2022

Copy link


Copy Markdown

Member
I guess we need to revisit and not close #331 .. at this point I agree the best option is set MSRV to rust 1.56, it's not the current stable for Debian.. but is being tested and should be shipped with next stable debian "bookworm" release. https://tracker.debian.org/pkg/rustc
@notmandatorynotmandatory
mentioned this pull request
Feb 17, 2022
 Closed

3 tasks
LLFournand others
added 2 commits
February 23, 2022 09:28
@LLFourn
Add API for internal addresses
00f0901
There are good reasons for applications to need to get internal
addresses too. For example creating a transactions that splits an output
into several smaller ones.
@LLFourn@rajarshimaitra
Fix comment on peek_address
022256c
Co-authored-by: Raj <36541669+rajarshimaitra@users.noreply.github.com>
@LLFourn
LLFourn
commented
Feb 22, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased. Should be ready for merge after CI (hopefully) passes.
notmandatory
notmandatory
approved these changes
Mar 2, 2022
View reviewed changes
@notmandatorynotmandatory
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

ACK 022256c
@notmandatory
notmandatory
dismissed
rajarshimaitra’s stale reviewMarch 2, 2022 04:48
Requested changes look to be resolved.
@bitcoindevkitbitcoindevkit
deleted a comment from codecovBotMar 2, 2022
@notmandatory
notmandatory
merged commit f0188f4
into
bitcoindevkit:masterMar 2, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@notmandatorynotmandatorynotmandatory approved these changes

+2 more reviewers

@w0xltw0xltw0xlt approved these changes
@rajarshimaitrarajarshimaitrarajarshimaitra left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@LLFournLLFourn

Labels


None yet


Projects


None yet


Milestone

Release 0.17.0 Feature Freeze

Development

Successfully merging this pull request may close these issues.

4 participants

@LLFourn@rajarshimaitra@notmandatory@w0xlt

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add API for internal addresses - #522
Merged
notmandatory merged 2 commits into
bitcoindevkit:masterfrom 
LLFourn:get_change_address
Mar 2, 2022
Merged
Add API for internal addresses#522
notmandatory merged 2 commits into
bitcoindevkit:masterfrom 
LLFourn:get_change_address
Conversation
@LLFourn
@LLFournLLFourn
commented
Jan 11, 2022

Copy link


Copy Markdown

Collaborator
There are good reasons for applications to need to get internal

addresses too. For example creating a transactions that splits an output

into several smaller ones.
Checklists
All Submissions:
  •  I've signed all my commits
  •  I followed the contribution guidelines
  •  I ran cargo fmt and cargo clippy before committing
New Features:
  •  I've added tests for the new feature
  •  I've added docs for the new feature
  •  I've updated CHANGELOG.md
@LLFournLLFourn
mentioned this pull request
Jan 11, 2022
 Closed

@LLFourn
LLFournforce-pushed
the
get_change_address
branch
2 times, most recently
from
1546c42 to
63f1ec0CompareJanuary 11, 2022 01:29
rajarshimaitra
rajarshimaitra
reviewed
Jan 11, 2022
View reviewed changes
@rajarshimaitrarajarshimaitra
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Concept + tACK 63f1ec0
I think it makes sense and the code looks good to me.
w0xlt
w0xlt
approved these changes
Jan 11, 2022
View reviewed changes
@w0xltw0xlt
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

tACK 63f1ec0
I tested this commit with two wallets:
1 - with internal and external descriptors: worked as expected.
2- with external descriptor only: get_internal_address returns the same addresses as get_address.
Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .
But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,
rajarshimaitra
rajarshimaitra
reviewed
Jan 12, 2022
View reviewed changes
Comment threadsrc/wallet/mod.rs
Outdated

@rajarshimaitra
rajarshimaitra
commented
Jan 12, 2022

Copy link


Copy Markdown

Contributor
Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .

But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,
I think the get_descriptor_for_keychain() is written that way to make wallet api agnostic of change descriptor. Weather its there or not, all the logic will simply follow "use external if there's no internal". This simplifies internal logic handling at many places and doesn't force us to handle "no change desc case" separately. and the lib user just keep this in mind while using the wallet. Its also documented in many places.
So it makes sense to me to keep the behavior intact in peek_address too.
@w0xltw0xlt
mentioned this pull request
Jan 13, 2022
 Closed

rajarshimaitra
rajarshimaitra
previously requested changes
Jan 14, 2022
View reviewed changes
@rajarshimaitrarajarshimaitra
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

ACK 89f88ce
Sorry I missed few others comment nits in last review, of the previous methods.
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
@LLFourn
LLFourn
commented
Jan 25, 2022

Copy link


Copy Markdown

CollaboratorAuthor
comments addressed ready for further review.
@notmandatory
notmandatory
commented
Feb 2, 2022•

edited

Loading

Copy link


Copy Markdown

Member
I think docs are breaking because of a regression in rust +nightly, there seems to be a bug fix PR in the works so I think we should wait a couple days and see if that fixes it. See #538
@notmandatory
notmandatory
commented
Feb 11, 2022

Copy link


Copy Markdown

Member
#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.
notmandatory
notmandatory
requested changes
Feb 11, 2022
View reviewed changes
@notmandatorynotmandatory
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good, one small thing to fix in the new test.
Comment threadsrc/wallet/mod.rs
Outdated

 .unwrap();

 assert_eq!(
 wallet.get_address(AddressIndex::New).unwrap().address,
@notmandatorynotmandatoryFeb 11, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Shouldn't you be verifying that the internal address is the same as the external here?

Suggested change

 wallet.get_address(AddressIndex::New).unwrap().address,
 wallet.get_internal_address(AddressIndex::New).unwrap().address,
@LLFournLLFournFeb 15, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

thanks fixed @notmandatory
@notmandatory
notmandatory
commented
Feb 15, 2022

Copy link


Copy Markdown

Member
#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.
I think you still need to rebase since this went in, that will fix the rust docs CI job.
@nickfarrownickfarrow
mentioned this pull request
Feb 16, 2022
 Closed

6 tasks
@LLFourn
LLFourn
commented
Feb 16, 2022•

edited

Loading

Copy link


Copy Markdown

CollaboratorAuthor
@notmandatory new CI problem. Looks like tokio increased MSRV without major version bump. I think we should set MSRV for bdk to v1.56 (2021 edition) and leave it there.
@notmandatory
notmandatory
commented
Feb 17, 2022

Copy link


Copy Markdown

Member
I guess we need to revisit and not close #331 .. at this point I agree the best option is set MSRV to rust 1.56, it's not the current stable for Debian.. but is being tested and should be shipped with next stable debian "bookworm" release. https://tracker.debian.org/pkg/rustc
@notmandatorynotmandatory
mentioned this pull request
Feb 17, 2022
 Closed

3 tasks
LLFournand others
added 2 commits
February 23, 2022 09:28
@LLFourn
Add API for internal addresses
00f0901
There are good reasons for applications to need to get internal
addresses too. For example creating a transactions that splits an output
into several smaller ones.
@LLFourn@rajarshimaitra
Fix comment on peek_address
022256c
Co-authored-by: Raj <36541669+rajarshimaitra@users.noreply.github.com>
@LLFourn
LLFourn
commented
Feb 22, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased. Should be ready for merge after CI (hopefully) passes.
notmandatory
notmandatory
approved these changes
Mar 2, 2022
View reviewed changes
@notmandatorynotmandatory
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

ACK 022256c
@notmandatory
notmandatory
dismissed
rajarshimaitra’s stale reviewMarch 2, 2022 04:48
Requested changes look to be resolved.
@bitcoindevkitbitcoindevkit
deleted a comment from codecovBotMar 2, 2022
@notmandatory
notmandatory
merged commit f0188f4
into
bitcoindevkit:masterMar 2, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@notmandatorynotmandatorynotmandatory approved these changes

+2 more reviewers

@w0xltw0xltw0xlt approved these changes
@rajarshimaitrarajarshimaitrarajarshimaitra left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@LLFournLLFourn

Labels


None yet


Projects


None yet


Milestone

Release 0.17.0 Feature Freeze

Development

Successfully merging this pull request may close these issues.

4 participants

@LLFourn@rajarshimaitra@notmandatory@w0xlt

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Add API for internal addresses - #522
Merged
notmandatory merged 2 commits into
bitcoindevkit:masterfrom 
LLFourn:get_change_address
Mar 2, 2022
Merged
Add API for internal addresses#522
notmandatory merged 2 commits into
bitcoindevkit:masterfrom 
LLFourn:get_change_address
Conversation
@LLFourn
@LLFournLLFourn
commented
Jan 11, 2022

Copy link


Copy Markdown

Collaborator
There are good reasons for applications to need to get internal

addresses too. For example creating a transactions that splits an output

into several smaller ones.
Checklists
All Submissions:
  •  I've signed all my commits
  •  I followed the contribution guidelines
  •  I ran cargo fmt and cargo clippy before committing
New Features:
  •  I've added tests for the new feature
  •  I've added docs for the new feature
  •  I've updated CHANGELOG.md
@LLFournLLFourn
mentioned this pull request
Jan 11, 2022
 Closed

@LLFourn
LLFournforce-pushed
the
get_change_address
branch
2 times, most recently
from
1546c42 to
63f1ec0CompareJanuary 11, 2022 01:29
rajarshimaitra
rajarshimaitra
reviewed
Jan 11, 2022
View reviewed changes
@rajarshimaitrarajarshimaitra
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Concept + tACK 63f1ec0
I think it makes sense and the code looks good to me.
w0xlt
w0xlt
approved these changes
Jan 11, 2022
View reviewed changes
@w0xltw0xlt
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

tACK 63f1ec0
I tested this commit with two wallets:
1 - with internal and external descriptors: worked as expected.
2- with external descriptor only: get_internal_address returns the same addresses as get_address.
Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .
But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,
rajarshimaitra
rajarshimaitra
reviewed
Jan 12, 2022
View reviewed changes
Comment threadsrc/wallet/mod.rs
Outdated

@rajarshimaitra
rajarshimaitra
commented
Jan 12, 2022

Copy link


Copy Markdown

Contributor
Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .

But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,
I think the get_descriptor_for_keychain() is written that way to make wallet api agnostic of change descriptor. Weather its there or not, all the logic will simply follow "use external if there's no internal". This simplifies internal logic handling at many places and doesn't force us to handle "no change desc case" separately. and the lib user just keep this in mind while using the wallet. Its also documented in many places.
So it makes sense to me to keep the behavior intact in peek_address too.
@w0xltw0xlt
mentioned this pull request
Jan 13, 2022
 Closed

rajarshimaitra
rajarshimaitra
previously requested changes
Jan 14, 2022
View reviewed changes
@rajarshimaitrarajarshimaitra
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

ACK 89f88ce
Sorry I missed few others comment nits in last review, of the previous methods.
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
@LLFourn
LLFourn
commented
Jan 25, 2022

Copy link


Copy Markdown

CollaboratorAuthor
comments addressed ready for further review.
@notmandatory
notmandatory
commented
Feb 2, 2022•

edited

Loading

Copy link


Copy Markdown

Member
I think docs are breaking because of a regression in rust +nightly, there seems to be a bug fix PR in the works so I think we should wait a couple days and see if that fixes it. See #538
@notmandatory
notmandatory
commented
Feb 11, 2022

Copy link


Copy Markdown

Member
#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.
notmandatory
notmandatory
requested changes
Feb 11, 2022
View reviewed changes
@notmandatorynotmandatory
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good, one small thing to fix in the new test.
Comment threadsrc/wallet/mod.rs
Outdated

 .unwrap();

 assert_eq!(
 wallet.get_address(AddressIndex::New).unwrap().address,
@notmandatorynotmandatoryFeb 11, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Shouldn't you be verifying that the internal address is the same as the external here?

Suggested change

 wallet.get_address(AddressIndex::New).unwrap().address,
 wallet.get_internal_address(AddressIndex::New).unwrap().address,
@LLFournLLFournFeb 15, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

thanks fixed @notmandatory
@notmandatory
notmandatory
commented
Feb 15, 2022

Copy link


Copy Markdown

Member
#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.
I think you still need to rebase since this went in, that will fix the rust docs CI job.
@nickfarrownickfarrow
mentioned this pull request
Feb 16, 2022
 Closed

6 tasks
@LLFourn
LLFourn
commented
Feb 16, 2022•

edited

Loading

Copy link


Copy Markdown

CollaboratorAuthor
@notmandatory new CI problem. Looks like tokio increased MSRV without major version bump. I think we should set MSRV for bdk to v1.56 (2021 edition) and leave it there.
@notmandatory
notmandatory
commented
Feb 17, 2022

Copy link


Copy Markdown

Member
I guess we need to revisit and not close #331 .. at this point I agree the best option is set MSRV to rust 1.56, it's not the current stable for Debian.. but is being tested and should be shipped with next stable debian "bookworm" release. https://tracker.debian.org/pkg/rustc
@notmandatorynotmandatory
mentioned this pull request
Feb 17, 2022
 Closed

3 tasks
LLFournand others
added 2 commits
February 23, 2022 09:28
@LLFourn
Add API for internal addresses
00f0901
There are good reasons for applications to need to get internal
addresses too. For example creating a transactions that splits an output
into several smaller ones.
@LLFourn@rajarshimaitra
Fix comment on peek_address
022256c
Co-authored-by: Raj <36541669+rajarshimaitra@users.noreply.github.com>
@LLFourn
LLFourn
commented
Feb 22, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased. Should be ready for merge after CI (hopefully) passes.
notmandatory
notmandatory
approved these changes
Mar 2, 2022
View reviewed changes
@notmandatorynotmandatory
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

ACK 022256c
@notmandatory
notmandatory
dismissed
rajarshimaitra’s stale reviewMarch 2, 2022 04:48
Requested changes look to be resolved.
@bitcoindevkitbitcoindevkit
deleted a comment from codecovBotMar 2, 2022
@notmandatory
notmandatory
merged commit f0188f4
into
bitcoindevkit:masterMar 2, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@notmandatorynotmandatorynotmandatory approved these changes

+2 more reviewers

@w0xltw0xltw0xlt approved these changes
@rajarshimaitrarajarshimaitrarajarshimaitra left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@LLFournLLFourn

Labels


None yet


Projects


None yet


Milestone

Release 0.17.0 Feature Freeze

Development

Successfully merging this pull request may close these issues.

4 participants

@LLFourn@rajarshimaitra@notmandatory@w0xlt

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add API for internal addresses - #522
Merged
notmandatory merged 2 commits into
bitcoindevkit:masterfrom 
LLFourn:get_change_address
Mar 2, 2022
Merged
Add API for internal addresses#522
notmandatory merged 2 commits into
bitcoindevkit:masterfrom 
LLFourn:get_change_address
Conversation
@LLFourn
@LLFournLLFourn
commented
Jan 11, 2022

Copy link


Copy Markdown

Collaborator
There are good reasons for applications to need to get internal

addresses too. For example creating a transactions that splits an output

into several smaller ones.
Checklists
All Submissions:
  •  I've signed all my commits
  •  I followed the contribution guidelines
  •  I ran cargo fmt and cargo clippy before committing
New Features:
  •  I've added tests for the new feature
  •  I've added docs for the new feature
  •  I've updated CHANGELOG.md
@LLFournLLFourn
mentioned this pull request
Jan 11, 2022
 Closed

@LLFourn
LLFournforce-pushed
the
get_change_address
branch
2 times, most recently
from
1546c42 to
63f1ec0CompareJanuary 11, 2022 01:29
rajarshimaitra
rajarshimaitra
reviewed
Jan 11, 2022
View reviewed changes
@rajarshimaitrarajarshimaitra
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Concept + tACK 63f1ec0
I think it makes sense and the code looks good to me.
w0xlt
w0xlt
approved these changes
Jan 11, 2022
View reviewed changes
@w0xltw0xlt
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

tACK 63f1ec0
I tested this commit with two wallets:
1 - with internal and external descriptors: worked as expected.
2- with external descriptor only: get_internal_address returns the same addresses as get_address.
Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .
But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,
rajarshimaitra
rajarshimaitra
reviewed
Jan 12, 2022
View reviewed changes
Comment threadsrc/wallet/mod.rs
Outdated

@rajarshimaitra
rajarshimaitra
commented
Jan 12, 2022

Copy link


Copy Markdown

Contributor
Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .

But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,
I think the get_descriptor_for_keychain() is written that way to make wallet api agnostic of change descriptor. Weather its there or not, all the logic will simply follow "use external if there's no internal". This simplifies internal logic handling at many places and doesn't force us to handle "no change desc case" separately. and the lib user just keep this in mind while using the wallet. Its also documented in many places.
So it makes sense to me to keep the behavior intact in peek_address too.
@w0xltw0xlt
mentioned this pull request
Jan 13, 2022
 Closed

rajarshimaitra
rajarshimaitra
previously requested changes
Jan 14, 2022
View reviewed changes
@rajarshimaitrarajarshimaitra
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

ACK 89f88ce
Sorry I missed few others comment nits in last review, of the previous methods.
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
@LLFourn
LLFourn
commented
Jan 25, 2022

Copy link


Copy Markdown

CollaboratorAuthor
comments addressed ready for further review.
@notmandatory
notmandatory
commented
Feb 2, 2022•

edited

Loading

Copy link


Copy Markdown

Member
I think docs are breaking because of a regression in rust +nightly, there seems to be a bug fix PR in the works so I think we should wait a couple days and see if that fixes it. See #538
@notmandatory
notmandatory
commented
Feb 11, 2022

Copy link


Copy Markdown

Member
#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.
notmandatory
notmandatory
requested changes
Feb 11, 2022
View reviewed changes
@notmandatorynotmandatory
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good, one small thing to fix in the new test.
Comment threadsrc/wallet/mod.rs
Outdated

 .unwrap();

 assert_eq!(
 wallet.get_address(AddressIndex::New).unwrap().address,
@notmandatorynotmandatoryFeb 11, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Shouldn't you be verifying that the internal address is the same as the external here?

Suggested change

 wallet.get_address(AddressIndex::New).unwrap().address,
 wallet.get_internal_address(AddressIndex::New).unwrap().address,
@LLFournLLFournFeb 15, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

thanks fixed @notmandatory
@notmandatory
notmandatory
commented
Feb 15, 2022

Copy link


Copy Markdown

Member
#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.
I think you still need to rebase since this went in, that will fix the rust docs CI job.
@nickfarrownickfarrow
mentioned this pull request
Feb 16, 2022
 Closed

6 tasks
@LLFourn
LLFourn
commented
Feb 16, 2022•

edited

Loading

Copy link


Copy Markdown

CollaboratorAuthor
@notmandatory new CI problem. Looks like tokio increased MSRV without major version bump. I think we should set MSRV for bdk to v1.56 (2021 edition) and leave it there.
@notmandatory
notmandatory
commented
Feb 17, 2022

Copy link


Copy Markdown

Member
I guess we need to revisit and not close #331 .. at this point I agree the best option is set MSRV to rust 1.56, it's not the current stable for Debian.. but is being tested and should be shipped with next stable debian "bookworm" release. https://tracker.debian.org/pkg/rustc
@notmandatorynotmandatory
mentioned this pull request
Feb 17, 2022
 Closed

3 tasks
LLFournand others
added 2 commits
February 23, 2022 09:28
@LLFourn
Add API for internal addresses
00f0901
There are good reasons for applications to need to get internal
addresses too. For example creating a transactions that splits an output
into several smaller ones.
@LLFourn@rajarshimaitra
Fix comment on peek_address
022256c
Co-authored-by: Raj <36541669+rajarshimaitra@users.noreply.github.com>
@LLFourn
LLFourn
commented
Feb 22, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased. Should be ready for merge after CI (hopefully) passes.
notmandatory
notmandatory
approved these changes
Mar 2, 2022
View reviewed changes
@notmandatorynotmandatory
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

ACK 022256c
@notmandatory
notmandatory
dismissed
rajarshimaitra’s stale reviewMarch 2, 2022 04:48
Requested changes look to be resolved.
@bitcoindevkitbitcoindevkit
deleted a comment from codecovBotMar 2, 2022
@notmandatory
notmandatory
merged commit f0188f4
into
bitcoindevkit:masterMar 2, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@notmandatorynotmandatorynotmandatory approved these changes

+2 more reviewers

@w0xltw0xltw0xlt approved these changes
@rajarshimaitrarajarshimaitrarajarshimaitra left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@LLFournLLFourn

Labels


None yet


Projects


None yet


Milestone

Release 0.17.0 Feature Freeze

Development

Successfully merging this pull request may close these issues.

4 participants

@LLFourn@rajarshimaitra@notmandatory@w0xlt

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add API for internal addresses - #522
Merged
notmandatory merged 2 commits into
bitcoindevkit:masterfrom 
LLFourn:get_change_address
Mar 2, 2022
Merged
Add API for internal addresses#522
notmandatory merged 2 commits into
bitcoindevkit:masterfrom 
LLFourn:get_change_address
Conversation
@LLFourn
@LLFournLLFourn
commented
Jan 11, 2022

Copy link


Copy Markdown

Collaborator
There are good reasons for applications to need to get internal

addresses too. For example creating a transactions that splits an output

into several smaller ones.
Checklists
All Submissions:
  •  I've signed all my commits
  •  I followed the contribution guidelines
  •  I ran cargo fmt and cargo clippy before committing
New Features:
  •  I've added tests for the new feature
  •  I've added docs for the new feature
  •  I've updated CHANGELOG.md
@LLFournLLFourn
mentioned this pull request
Jan 11, 2022
 Closed

@LLFourn
LLFournforce-pushed
the
get_change_address
branch
2 times, most recently
from
1546c42 to
63f1ec0CompareJanuary 11, 2022 01:29
rajarshimaitra
rajarshimaitra
reviewed
Jan 11, 2022
View reviewed changes
@rajarshimaitrarajarshimaitra
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Concept + tACK 63f1ec0
I think it makes sense and the code looks good to me.
w0xlt
w0xlt
approved these changes
Jan 11, 2022
View reviewed changes
@w0xltw0xlt
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

tACK 63f1ec0
I tested this commit with two wallets:
1 - with internal and external descriptors: worked as expected.
2- with external descriptor only: get_internal_address returns the same addresses as get_address.
Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .
But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,
rajarshimaitra
rajarshimaitra
reviewed
Jan 12, 2022
View reviewed changes
Comment threadsrc/wallet/mod.rs
Outdated

@rajarshimaitra
rajarshimaitra
commented
Jan 12, 2022

Copy link


Copy Markdown

Contributor
Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .

But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,
I think the get_descriptor_for_keychain() is written that way to make wallet api agnostic of change descriptor. Weather its there or not, all the logic will simply follow "use external if there's no internal". This simplifies internal logic handling at many places and doesn't force us to handle "no change desc case" separately. and the lib user just keep this in mind while using the wallet. Its also documented in many places.
So it makes sense to me to keep the behavior intact in peek_address too.
@w0xltw0xlt
mentioned this pull request
Jan 13, 2022
 Closed

rajarshimaitra
rajarshimaitra
previously requested changes
Jan 14, 2022
View reviewed changes
@rajarshimaitrarajarshimaitra
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

ACK 89f88ce
Sorry I missed few others comment nits in last review, of the previous methods.
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
@LLFourn
LLFourn
commented
Jan 25, 2022

Copy link


Copy Markdown

CollaboratorAuthor
comments addressed ready for further review.
@notmandatory
notmandatory
commented
Feb 2, 2022•

edited

Loading

Copy link


Copy Markdown

Member
I think docs are breaking because of a regression in rust +nightly, there seems to be a bug fix PR in the works so I think we should wait a couple days and see if that fixes it. See #538
@notmandatory
notmandatory
commented
Feb 11, 2022

Copy link


Copy Markdown

Member
#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.
notmandatory
notmandatory
requested changes
Feb 11, 2022
View reviewed changes
@notmandatorynotmandatory
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good, one small thing to fix in the new test.
Comment threadsrc/wallet/mod.rs
Outdated

 .unwrap();

 assert_eq!(
 wallet.get_address(AddressIndex::New).unwrap().address,
@notmandatorynotmandatoryFeb 11, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Shouldn't you be verifying that the internal address is the same as the external here?

Suggested change

 wallet.get_address(AddressIndex::New).unwrap().address,
 wallet.get_internal_address(AddressIndex::New).unwrap().address,
@LLFournLLFournFeb 15, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

thanks fixed @notmandatory
@notmandatory
notmandatory
commented
Feb 15, 2022

Copy link


Copy Markdown

Member
#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.
I think you still need to rebase since this went in, that will fix the rust docs CI job.
@nickfarrownickfarrow
mentioned this pull request
Feb 16, 2022
 Closed

6 tasks
@LLFourn
LLFourn
commented
Feb 16, 2022•

edited

Loading

Copy link


Copy Markdown

CollaboratorAuthor
@notmandatory new CI problem. Looks like tokio increased MSRV without major version bump. I think we should set MSRV for bdk to v1.56 (2021 edition) and leave it there.
@notmandatory
notmandatory
commented
Feb 17, 2022

Copy link


Copy Markdown

Member
I guess we need to revisit and not close #331 .. at this point I agree the best option is set MSRV to rust 1.56, it's not the current stable for Debian.. but is being tested and should be shipped with next stable debian "bookworm" release. https://tracker.debian.org/pkg/rustc
@notmandatorynotmandatory
mentioned this pull request
Feb 17, 2022
 Closed

3 tasks
LLFournand others
added 2 commits
February 23, 2022 09:28
@LLFourn
Add API for internal addresses
00f0901
There are good reasons for applications to need to get internal
addresses too. For example creating a transactions that splits an output
into several smaller ones.
@LLFourn@rajarshimaitra
Fix comment on peek_address
022256c
Co-authored-by: Raj <36541669+rajarshimaitra@users.noreply.github.com>
@LLFourn
LLFourn
commented
Feb 22, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased. Should be ready for merge after CI (hopefully) passes.
notmandatory
notmandatory
approved these changes
Mar 2, 2022
View reviewed changes
@notmandatorynotmandatory
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

ACK 022256c
@notmandatory
notmandatory
dismissed
rajarshimaitra’s stale reviewMarch 2, 2022 04:48
Requested changes look to be resolved.
@bitcoindevkitbitcoindevkit
deleted a comment from codecovBotMar 2, 2022
@notmandatory
notmandatory
merged commit f0188f4
into
bitcoindevkit:masterMar 2, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@notmandatorynotmandatorynotmandatory approved these changes

+2 more reviewers

@w0xltw0xltw0xlt approved these changes
@rajarshimaitrarajarshimaitrarajarshimaitra left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@LLFournLLFourn

Labels


None yet


Projects


None yet


Milestone

Release 0.17.0 Feature Freeze

Development

Successfully merging this pull request may close these issues.

4 participants

@LLFourn@rajarshimaitra@notmandatory@w0xlt

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Add API for internal addresses - #522
Merged
notmandatory merged 2 commits into
bitcoindevkit:masterfrom 
LLFourn:get_change_address
Mar 2, 2022
Merged
Add API for internal addresses#522
notmandatory merged 2 commits into
bitcoindevkit:masterfrom 
LLFourn:get_change_address
Conversation
@LLFourn
@LLFournLLFourn
commented
Jan 11, 2022

Copy link


Copy Markdown

Collaborator
There are good reasons for applications to need to get internal

addresses too. For example creating a transactions that splits an output

into several smaller ones.
Checklists
All Submissions:
  •  I've signed all my commits
  •  I followed the contribution guidelines
  •  I ran cargo fmt and cargo clippy before committing
New Features:
  •  I've added tests for the new feature
  •  I've added docs for the new feature
  •  I've updated CHANGELOG.md
@LLFournLLFourn
mentioned this pull request
Jan 11, 2022
 Closed

@LLFourn
LLFournforce-pushed
the
get_change_address
branch
2 times, most recently
from
1546c42 to
63f1ec0CompareJanuary 11, 2022 01:29
rajarshimaitra
rajarshimaitra
reviewed
Jan 11, 2022
View reviewed changes
@rajarshimaitrarajarshimaitra
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Concept + tACK 63f1ec0
I think it makes sense and the code looks good to me.
w0xlt
w0xlt
approved these changes
Jan 11, 2022
View reviewed changes
@w0xltw0xlt
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

tACK 63f1ec0
I tested this commit with two wallets:
1 - with internal and external descriptors: worked as expected.
2- with external descriptor only: get_internal_address returns the same addresses as get_address.
Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .
But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,
rajarshimaitra
rajarshimaitra
reviewed
Jan 12, 2022
View reviewed changes
Comment threadsrc/wallet/mod.rs
Outdated

@rajarshimaitra
rajarshimaitra
commented
Jan 12, 2022

Copy link


Copy Markdown

Contributor
Maybe if there is not an internal descriptor, a possible better approach would be to throw an error when calling get_internal_address .

But I think this behavior is not related to this PR. It happens because get_descriptor_for_keychain(keychain) apparently returns the same descriptor for internal and external keychain when the wallet is created with external descriptor only,
I think the get_descriptor_for_keychain() is written that way to make wallet api agnostic of change descriptor. Weather its there or not, all the logic will simply follow "use external if there's no internal". This simplifies internal logic handling at many places and doesn't force us to handle "no change desc case" separately. and the lib user just keep this in mind while using the wallet. Its also documented in many places.
So it makes sense to me to keep the behavior intact in peek_address too.
@w0xltw0xlt
mentioned this pull request
Jan 13, 2022
 Closed

rajarshimaitra
rajarshimaitra
previously requested changes
Jan 14, 2022
View reviewed changes
@rajarshimaitrarajarshimaitra
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

ACK 89f88ce
Sorry I missed few others comment nits in last review, of the previous methods.
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs
@LLFourn
LLFourn
commented
Jan 25, 2022

Copy link


Copy Markdown

CollaboratorAuthor
comments addressed ready for further review.
@notmandatory
notmandatory
commented
Feb 2, 2022•

edited

Loading

Copy link


Copy Markdown

Member
I think docs are breaking because of a regression in rust +nightly, there seems to be a bug fix PR in the works so I think we should wait a couple days and see if that fixes it. See #538
@notmandatory
notmandatory
commented
Feb 11, 2022

Copy link


Copy Markdown

Member
#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.
notmandatory
notmandatory
requested changes
Feb 11, 2022
View reviewed changes
@notmandatorynotmandatory
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good, one small thing to fix in the new test.
Comment threadsrc/wallet/mod.rs
Outdated

 .unwrap();

 assert_eq!(
 wallet.get_address(AddressIndex::New).unwrap().address,
@notmandatorynotmandatoryFeb 11, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Shouldn't you be verifying that the internal address is the same as the external here?

Suggested change

 wallet.get_address(AddressIndex::New).unwrap().address,
 wallet.get_internal_address(AddressIndex::New).unwrap().address,
@LLFournLLFournFeb 15, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

thanks fixed @notmandatory
@notmandatory
notmandatory
commented
Feb 15, 2022

Copy link


Copy Markdown

Member
#538 is merged and fixes the docs issue, once you rebase this PR should be good to go.
I think you still need to rebase since this went in, that will fix the rust docs CI job.
@nickfarrownickfarrow
mentioned this pull request
Feb 16, 2022
 Closed

6 tasks
@LLFourn
LLFourn
commented
Feb 16, 2022•

edited

Loading

Copy link


Copy Markdown

CollaboratorAuthor
@notmandatory new CI problem. Looks like tokio increased MSRV without major version bump. I think we should set MSRV for bdk to v1.56 (2021 edition) and leave it there.
@notmandatory
notmandatory
commented
Feb 17, 2022

Copy link


Copy Markdown

Member
I guess we need to revisit and not close #331 .. at this point I agree the best option is set MSRV to rust 1.56, it's not the current stable for Debian.. but is being tested and should be shipped with next stable debian "bookworm" release. https://tracker.debian.org/pkg/rustc
@notmandatorynotmandatory
mentioned this pull request
Feb 17, 2022
 Closed

3 tasks
LLFournand others
added 2 commits
February 23, 2022 09:28
@LLFourn
Add API for internal addresses
00f0901
There are good reasons for applications to need to get internal
addresses too. For example creating a transactions that splits an output
into several smaller ones.
@LLFourn@rajarshimaitra
Fix comment on peek_address
022256c
Co-authored-by: Raj <36541669+rajarshimaitra@users.noreply.github.com>
@LLFourn
LLFourn
commented
Feb 22, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased. Should be ready for merge after CI (hopefully) passes.
notmandatory
notmandatory
approved these changes
Mar 2, 2022
View reviewed changes
@notmandatorynotmandatory
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

ACK 022256c
@notmandatory
notmandatory
dismissed
rajarshimaitra’s stale reviewMarch 2, 2022 04:48
Requested changes look to be resolved.
@bitcoindevkitbitcoindevkit
deleted a comment from codecovBotMar 2, 2022
@notmandatory
notmandatory
merged commit f0188f4
into
bitcoindevkit:masterMar 2, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@notmandatorynotmandatorynotmandatory approved these changes

+2 more reviewers

@w0xltw0xltw0xlt approved these changes
@rajarshimaitrarajarshimaitrarajarshimaitra left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@LLFournLLFourn

Labels


None yet


Projects


None yet


Milestone

Release 0.17.0 Feature Freeze

Development

Successfully merging this pull request may close these issues.

4 participants

@LLFourn@rajarshimaitra@notmandatory@w0xlt