Get balance in categories by wszdexdrf · Pull Request #640 · bitcoindevkit/bdk

wszdexdrf · 2022-06-23T15:43:44Z

Description

This changes get_balance() function so that it returns balance separated in 4 categories:

available
trusted-pending
untrusted-pending
immature

Fixes#238

Notes to the reviewers

Based on #614

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 updated tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

wszdexdrf · 2022-06-23T15:46:23Z

I have not added tests since I feel that existing test should be fine. I can add some if need be.
Regarding the code, I went with the rule "add instead of change"

danielabrozzoni

Concept ACK

Some tests are failing, run them with cargo test --features test-electrum and cargo test --features test-rpc
You should update the CHANGELOG.md
You should add tests! Some ideas:
- Receive a transaction, check that the output values get counted in the untrusted_pending, get it confirmed, check that now the amount is in available
- Make a transaction, check that the change value is counted in trusted_pending, get it confirmed, check that now the amount is in available

danielabrozzoni · 2022-06-28T08:29:05Z

Can you rebase to pick up the CI fixes (#638)? Thanks :)

evanlinjin

Concept ACK

I'm very excited for these changes, and great work!

I just have some thoughts on naming conventions and some changes in logic. Haven't really gone through everything yet though.

In regards to naming conventions, may need various second opinions.

evanlinjin · 2022-07-05T06:22:59Z

+#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Default)]
+pub struct Balance {
+ /// All coinbase outputs not yet matured
+ pub immature: u64,


Would immature_coinbase be clearer? Or too wordy? 😅

I feel immature coins are only ever meant in regards to coinbase coins, so I think it would be a bit too wordy. I thought of these names as if they had a imaginary "balance" or "coins" added at the end of them.

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?
Not at the moment! But I should open an issue to remember this. Also see #640 (comment)

......which means that maybe immature coinbase is clearer...

afilini · 2022-07-05T20:35:46Z

FYI: #614 got merged, so you can rebase on master to pickup the latest changes.

It also looks like there are some conflicts, so during the rebase you'll get a chance to fix them as well.

EDIT: I think the conflicts mostly come from the two commits you have in your branch from the old version of #614, so just dropping those and rebasing on master should fix everything.

danielabrozzoni

tACK 74def0a

afilini

I hate to make you go through another round of fixing and rebasing but I swear, this is gonna be the last one!

The code looked great to me now, no complains there.

afilini

ACK 62e6b04

evanlinjin

ACK 62e6b04

However, just a thought...

For tests that require checking balance, would it make sense to compare with the whole Balance struct?

For example:

assert_eq!(wallet.get_balance().unwrap(),Balance{ .. })

But I hate to be the one that delays merging, maybe we can have this as another task for another PR?

Otherwise, I've gone through everything and it all looks dandy to me :)

danielabrozzoni · 2022-07-20T13:57:55Z

This needs rebasing to fix the conflicts.

One small note: please, when you've updated the code, can you click "resolve conversation" under the comments? This way the whole PR is way more readable. I've done it for you this time, I hope I didn't accidentally solve something that wasn't solved...

danielabrozzoni

Just a tiny nit. Me and Alekos have been recently talking about lacking test coverage (mainly after we discovered #666 😈 ), and we realize that we should definitely be more nit-picker on tests. Sorry to delay merging even more, but I promise we're close!

Add type balance with add, display traits. Change affected tests. Update `CHANGELOG.md`

danielabrozzoni · 2022-08-04T08:39:38Z

Rebased

afilini

ACK 0f03831

3c6075a Add Balance struct and conversion from BdkBalance (thunderbiscuit) 4e15bad Update BDK to version 0.22 (thunderbiscuit) Pull request description: The bindings do not build when attempting this upgrade because `get_balance()` now returns a `Balance` struct (this was merged in bitcoindevkit/bdk#640) ```sh error[E0308]: mismatched types --> src/lib.rs:433:9 | 432 | fn get_balance(&self) -> Result<u64, Error> { | ------------------ expected `Result<u64, bdk::Error>` because of return type 433 | self.get_wallet().get_balance() | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u64`, found struct `Balance` | = note: expected enum `Result<u64, _>` found enum `Result<Balance, _>` For more information about this error, try `rustc --explain E0308`. error: could not compile `bdk-ffi` due to previous error ``` When we upgrade to `0.22.0` we could decide to add the `Balance` struct to the bindings, or simply return the total by calling `get_total()`, which returns a `u64` (same as we have now). ACKs for top commit: notmandatory: ACK 3c6075a Tree-SHA512: 13d2f83f992735f4f9619ae339d7834df08385129edf06bac830c298b433571af3f211e92a6da1f4f9646dec27dbd2c6133a035f26eac8757b7a1c94b54b463d

wszdexdrf · 2022-06-23T15:43:44Z

Description

This changes get_balance() function so that it returns balance separated in 4 categories:

available
trusted-pending
untrusted-pending
immature

Fixes#238

Notes to the reviewers

Based on #614

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 updated tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

wszdexdrf · 2022-06-23T15:46:23Z

I have not added tests since I feel that existing test should be fine. I can add some if need be.
Regarding the code, I went with the rule "add instead of change"

danielabrozzoni

Concept ACK

Some tests are failing, run them with cargo test --features test-electrum and cargo test --features test-rpc
You should update the CHANGELOG.md
You should add tests! Some ideas:
- Receive a transaction, check that the output values get counted in the untrusted_pending, get it confirmed, check that now the amount is in available
- Make a transaction, check that the change value is counted in trusted_pending, get it confirmed, check that now the amount is in available

danielabrozzoni · 2022-06-28T08:29:05Z

Can you rebase to pick up the CI fixes (#638)? Thanks :)

evanlinjin

Concept ACK

I'm very excited for these changes, and great work!

I just have some thoughts on naming conventions and some changes in logic. Haven't really gone through everything yet though.

In regards to naming conventions, may need various second opinions.

evanlinjin · 2022-07-05T06:22:59Z

+#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Default)]
+pub struct Balance {
+ /// All coinbase outputs not yet matured
+ pub immature: u64,


Would immature_coinbase be clearer? Or too wordy? 😅

I feel immature coins are only ever meant in regards to coinbase coins, so I think it would be a bit too wordy. I thought of these names as if they had a imaginary "balance" or "coins" added at the end of them.

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?
Not at the moment! But I should open an issue to remember this. Also see #640 (comment)

......which means that maybe immature coinbase is clearer...

afilini · 2022-07-05T20:35:46Z

FYI: #614 got merged, so you can rebase on master to pickup the latest changes.

It also looks like there are some conflicts, so during the rebase you'll get a chance to fix them as well.

EDIT: I think the conflicts mostly come from the two commits you have in your branch from the old version of #614, so just dropping those and rebasing on master should fix everything.

danielabrozzoni

tACK 74def0a

afilini

I hate to make you go through another round of fixing and rebasing but I swear, this is gonna be the last one!

The code looked great to me now, no complains there.

afilini

ACK 62e6b04

evanlinjin

ACK 62e6b04

However, just a thought...

For tests that require checking balance, would it make sense to compare with the whole Balance struct?

For example:

assert_eq!(wallet.get_balance().unwrap(),Balance{ .. })

But I hate to be the one that delays merging, maybe we can have this as another task for another PR?

Otherwise, I've gone through everything and it all looks dandy to me :)

danielabrozzoni · 2022-07-20T13:57:55Z

This needs rebasing to fix the conflicts.

One small note: please, when you've updated the code, can you click "resolve conversation" under the comments? This way the whole PR is way more readable. I've done it for you this time, I hope I didn't accidentally solve something that wasn't solved...

danielabrozzoni

Just a tiny nit. Me and Alekos have been recently talking about lacking test coverage (mainly after we discovered #666 😈 ), and we realize that we should definitely be more nit-picker on tests. Sorry to delay merging even more, but I promise we're close!

Add type balance with add, display traits. Change affected tests. Update `CHANGELOG.md`

danielabrozzoni · 2022-08-04T08:39:38Z

Rebased

afilini

ACK 0f03831

3c6075a Add Balance struct and conversion from BdkBalance (thunderbiscuit) 4e15bad Update BDK to version 0.22 (thunderbiscuit) Pull request description: The bindings do not build when attempting this upgrade because `get_balance()` now returns a `Balance` struct (this was merged in bitcoindevkit/bdk#640) ```sh error[E0308]: mismatched types --> src/lib.rs:433:9 | 432 | fn get_balance(&self) -> Result<u64, Error> { | ------------------ expected `Result<u64, bdk::Error>` because of return type 433 | self.get_wallet().get_balance() | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u64`, found struct `Balance` | = note: expected enum `Result<u64, _>` found enum `Result<Balance, _>` For more information about this error, try `rustc --explain E0308`. error: could not compile `bdk-ffi` due to previous error ``` When we upgrade to `0.22.0` we could decide to add the `Balance` struct to the bindings, or simply return the total by calling `get_total()`, which returns a `u64` (same as we have now). ACKs for top commit: notmandatory: ACK 3c6075a Tree-SHA512: 13d2f83f992735f4f9619ae339d7834df08385129edf06bac830c298b433571af3f211e92a6da1f4f9646dec27dbd2c6133a035f26eac8757b7a1c94b54b463d

wszdexdrf · 2022-06-23T15:43:44Z

Description

This changes get_balance() function so that it returns balance separated in 4 categories:

available
trusted-pending
untrusted-pending
immature

Fixes#238

Notes to the reviewers

Based on #614

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 updated tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

wszdexdrf · 2022-06-23T15:46:23Z

I have not added tests since I feel that existing test should be fine. I can add some if need be.
Regarding the code, I went with the rule "add instead of change"

danielabrozzoni

Concept ACK

Some tests are failing, run them with cargo test --features test-electrum and cargo test --features test-rpc
You should update the CHANGELOG.md
You should add tests! Some ideas:
- Receive a transaction, check that the output values get counted in the untrusted_pending, get it confirmed, check that now the amount is in available
- Make a transaction, check that the change value is counted in trusted_pending, get it confirmed, check that now the amount is in available

danielabrozzoni · 2022-06-28T08:29:05Z

Can you rebase to pick up the CI fixes (#638)? Thanks :)

evanlinjin

Concept ACK

I'm very excited for these changes, and great work!

I just have some thoughts on naming conventions and some changes in logic. Haven't really gone through everything yet though.

In regards to naming conventions, may need various second opinions.

evanlinjin · 2022-07-05T06:22:59Z

+#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Default)]
+pub struct Balance {
+ /// All coinbase outputs not yet matured
+ pub immature: u64,


Would immature_coinbase be clearer? Or too wordy? 😅

I feel immature coins are only ever meant in regards to coinbase coins, so I think it would be a bit too wordy. I thought of these names as if they had a imaginary "balance" or "coins" added at the end of them.

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?
Not at the moment! But I should open an issue to remember this. Also see #640 (comment)

......which means that maybe immature coinbase is clearer...

afilini · 2022-07-05T20:35:46Z

FYI: #614 got merged, so you can rebase on master to pickup the latest changes.

It also looks like there are some conflicts, so during the rebase you'll get a chance to fix them as well.

EDIT: I think the conflicts mostly come from the two commits you have in your branch from the old version of #614, so just dropping those and rebasing on master should fix everything.

danielabrozzoni

tACK 74def0a

afilini

I hate to make you go through another round of fixing and rebasing but I swear, this is gonna be the last one!

The code looked great to me now, no complains there.

afilini

ACK 62e6b04

evanlinjin

ACK 62e6b04

However, just a thought...

For tests that require checking balance, would it make sense to compare with the whole Balance struct?

For example:

assert_eq!(wallet.get_balance().unwrap(),Balance{ .. })

But I hate to be the one that delays merging, maybe we can have this as another task for another PR?

Otherwise, I've gone through everything and it all looks dandy to me :)

danielabrozzoni · 2022-07-20T13:57:55Z

This needs rebasing to fix the conflicts.

One small note: please, when you've updated the code, can you click "resolve conversation" under the comments? This way the whole PR is way more readable. I've done it for you this time, I hope I didn't accidentally solve something that wasn't solved...

danielabrozzoni

Just a tiny nit. Me and Alekos have been recently talking about lacking test coverage (mainly after we discovered #666 😈 ), and we realize that we should definitely be more nit-picker on tests. Sorry to delay merging even more, but I promise we're close!

Add type balance with add, display traits. Change affected tests. Update `CHANGELOG.md`

danielabrozzoni · 2022-08-04T08:39:38Z

Rebased

afilini

ACK 0f03831

3c6075a Add Balance struct and conversion from BdkBalance (thunderbiscuit) 4e15bad Update BDK to version 0.22 (thunderbiscuit) Pull request description: The bindings do not build when attempting this upgrade because `get_balance()` now returns a `Balance` struct (this was merged in bitcoindevkit/bdk#640) ```sh error[E0308]: mismatched types --> src/lib.rs:433:9 | 432 | fn get_balance(&self) -> Result<u64, Error> { | ------------------ expected `Result<u64, bdk::Error>` because of return type 433 | self.get_wallet().get_balance() | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u64`, found struct `Balance` | = note: expected enum `Result<u64, _>` found enum `Result<Balance, _>` For more information about this error, try `rustc --explain E0308`. error: could not compile `bdk-ffi` due to previous error ``` When we upgrade to `0.22.0` we could decide to add the `Balance` struct to the bindings, or simply return the total by calling `get_total()`, which returns a `u64` (same as we have now). ACKs for top commit: notmandatory: ACK 3c6075a Tree-SHA512: 13d2f83f992735f4f9619ae339d7834df08385129edf06bac830c298b433571af3f211e92a6da1f4f9646dec27dbd2c6133a035f26eac8757b7a1c94b54b463d

wszdexdrf · 2022-06-23T15:43:44Z

Description

This changes get_balance() function so that it returns balance separated in 4 categories:

available
trusted-pending
untrusted-pending
immature

Fixes#238

Notes to the reviewers

Based on #614

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 updated tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

wszdexdrf · 2022-06-23T15:46:23Z

I have not added tests since I feel that existing test should be fine. I can add some if need be.
Regarding the code, I went with the rule "add instead of change"

danielabrozzoni

Concept ACK

Some tests are failing, run them with cargo test --features test-electrum and cargo test --features test-rpc
You should update the CHANGELOG.md
You should add tests! Some ideas:
- Receive a transaction, check that the output values get counted in the untrusted_pending, get it confirmed, check that now the amount is in available
- Make a transaction, check that the change value is counted in trusted_pending, get it confirmed, check that now the amount is in available

danielabrozzoni · 2022-06-28T08:29:05Z

Can you rebase to pick up the CI fixes (#638)? Thanks :)

evanlinjin

Concept ACK

I'm very excited for these changes, and great work!

I just have some thoughts on naming conventions and some changes in logic. Haven't really gone through everything yet though.

In regards to naming conventions, may need various second opinions.

evanlinjin · 2022-07-05T06:22:59Z

+#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Default)]
+pub struct Balance {
+ /// All coinbase outputs not yet matured
+ pub immature: u64,


Would immature_coinbase be clearer? Or too wordy? 😅

I feel immature coins are only ever meant in regards to coinbase coins, so I think it would be a bit too wordy. I thought of these names as if they had a imaginary "balance" or "coins" added at the end of them.

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?
Not at the moment! But I should open an issue to remember this. Also see #640 (comment)

......which means that maybe immature coinbase is clearer...

afilini · 2022-07-05T20:35:46Z

FYI: #614 got merged, so you can rebase on master to pickup the latest changes.

It also looks like there are some conflicts, so during the rebase you'll get a chance to fix them as well.

EDIT: I think the conflicts mostly come from the two commits you have in your branch from the old version of #614, so just dropping those and rebasing on master should fix everything.

danielabrozzoni

tACK 74def0a

afilini

I hate to make you go through another round of fixing and rebasing but I swear, this is gonna be the last one!

The code looked great to me now, no complains there.

afilini

ACK 62e6b04

evanlinjin

ACK 62e6b04

However, just a thought...

For tests that require checking balance, would it make sense to compare with the whole Balance struct?

For example:

assert_eq!(wallet.get_balance().unwrap(),Balance{ .. })

But I hate to be the one that delays merging, maybe we can have this as another task for another PR?

Otherwise, I've gone through everything and it all looks dandy to me :)

danielabrozzoni · 2022-07-20T13:57:55Z

This needs rebasing to fix the conflicts.

One small note: please, when you've updated the code, can you click "resolve conversation" under the comments? This way the whole PR is way more readable. I've done it for you this time, I hope I didn't accidentally solve something that wasn't solved...

danielabrozzoni

Just a tiny nit. Me and Alekos have been recently talking about lacking test coverage (mainly after we discovered #666 😈 ), and we realize that we should definitely be more nit-picker on tests. Sorry to delay merging even more, but I promise we're close!

Add type balance with add, display traits. Change affected tests. Update `CHANGELOG.md`

danielabrozzoni · 2022-08-04T08:39:38Z

Rebased

afilini

ACK 0f03831

3c6075a Add Balance struct and conversion from BdkBalance (thunderbiscuit) 4e15bad Update BDK to version 0.22 (thunderbiscuit) Pull request description: The bindings do not build when attempting this upgrade because `get_balance()` now returns a `Balance` struct (this was merged in bitcoindevkit/bdk#640) ```sh error[E0308]: mismatched types --> src/lib.rs:433:9 | 432 | fn get_balance(&self) -> Result<u64, Error> { | ------------------ expected `Result<u64, bdk::Error>` because of return type 433 | self.get_wallet().get_balance() | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u64`, found struct `Balance` | = note: expected enum `Result<u64, _>` found enum `Result<Balance, _>` For more information about this error, try `rustc --explain E0308`. error: could not compile `bdk-ffi` due to previous error ``` When we upgrade to `0.22.0` we could decide to add the `Balance` struct to the bindings, or simply return the total by calling `get_total()`, which returns a `u64` (same as we have now). ACKs for top commit: notmandatory: ACK 3c6075a Tree-SHA512: 13d2f83f992735f4f9619ae339d7834df08385129edf06bac830c298b433571af3f211e92a6da1f4f9646dec27dbd2c6133a035f26eac8757b7a1c94b54b463d

wszdexdrf · 2022-06-23T15:43:44Z

Description

This changes get_balance() function so that it returns balance separated in 4 categories:

available
trusted-pending
untrusted-pending
immature

Fixes#238

Notes to the reviewers

Based on #614

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 updated tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

wszdexdrf · 2022-06-23T15:46:23Z

I have not added tests since I feel that existing test should be fine. I can add some if need be.
Regarding the code, I went with the rule "add instead of change"

danielabrozzoni

Concept ACK

Some tests are failing, run them with cargo test --features test-electrum and cargo test --features test-rpc
You should update the CHANGELOG.md
You should add tests! Some ideas:
- Receive a transaction, check that the output values get counted in the untrusted_pending, get it confirmed, check that now the amount is in available
- Make a transaction, check that the change value is counted in trusted_pending, get it confirmed, check that now the amount is in available

danielabrozzoni · 2022-06-28T08:29:05Z

Can you rebase to pick up the CI fixes (#638)? Thanks :)

evanlinjin

Concept ACK

I'm very excited for these changes, and great work!

I just have some thoughts on naming conventions and some changes in logic. Haven't really gone through everything yet though.

In regards to naming conventions, may need various second opinions.

evanlinjin · 2022-07-05T06:22:59Z

+#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Default)]
+pub struct Balance {
+ /// All coinbase outputs not yet matured
+ pub immature: u64,


Would immature_coinbase be clearer? Or too wordy? 😅

I feel immature coins are only ever meant in regards to coinbase coins, so I think it would be a bit too wordy. I thought of these names as if they had a imaginary "balance" or "coins" added at the end of them.

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?
Not at the moment! But I should open an issue to remember this. Also see #640 (comment)

......which means that maybe immature coinbase is clearer...

afilini · 2022-07-05T20:35:46Z

FYI: #614 got merged, so you can rebase on master to pickup the latest changes.

It also looks like there are some conflicts, so during the rebase you'll get a chance to fix them as well.

EDIT: I think the conflicts mostly come from the two commits you have in your branch from the old version of #614, so just dropping those and rebasing on master should fix everything.

danielabrozzoni

tACK 74def0a

afilini

I hate to make you go through another round of fixing and rebasing but I swear, this is gonna be the last one!

The code looked great to me now, no complains there.

afilini

ACK 62e6b04

evanlinjin

ACK 62e6b04

However, just a thought...

For tests that require checking balance, would it make sense to compare with the whole Balance struct?

For example:

assert_eq!(wallet.get_balance().unwrap(),Balance{ .. })

But I hate to be the one that delays merging, maybe we can have this as another task for another PR?

Otherwise, I've gone through everything and it all looks dandy to me :)

danielabrozzoni · 2022-07-20T13:57:55Z

This needs rebasing to fix the conflicts.

One small note: please, when you've updated the code, can you click "resolve conversation" under the comments? This way the whole PR is way more readable. I've done it for you this time, I hope I didn't accidentally solve something that wasn't solved...

danielabrozzoni

Just a tiny nit. Me and Alekos have been recently talking about lacking test coverage (mainly after we discovered #666 😈 ), and we realize that we should definitely be more nit-picker on tests. Sorry to delay merging even more, but I promise we're close!

Add type balance with add, display traits. Change affected tests. Update `CHANGELOG.md`

danielabrozzoni · 2022-08-04T08:39:38Z

Rebased

afilini

ACK 0f03831

3c6075a Add Balance struct and conversion from BdkBalance (thunderbiscuit) 4e15bad Update BDK to version 0.22 (thunderbiscuit) Pull request description: The bindings do not build when attempting this upgrade because `get_balance()` now returns a `Balance` struct (this was merged in bitcoindevkit/bdk#640) ```sh error[E0308]: mismatched types --> src/lib.rs:433:9 | 432 | fn get_balance(&self) -> Result<u64, Error> { | ------------------ expected `Result<u64, bdk::Error>` because of return type 433 | self.get_wallet().get_balance() | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u64`, found struct `Balance` | = note: expected enum `Result<u64, _>` found enum `Result<Balance, _>` For more information about this error, try `rustc --explain E0308`. error: could not compile `bdk-ffi` due to previous error ``` When we upgrade to `0.22.0` we could decide to add the `Balance` struct to the bindings, or simply return the total by calling `get_total()`, which returns a `u64` (same as we have now). ACKs for top commit: notmandatory: ACK 3c6075a Tree-SHA512: 13d2f83f992735f4f9619ae339d7834df08385129edf06bac830c298b433571af3f211e92a6da1f4f9646dec27dbd2c6133a035f26eac8757b7a1c94b54b463d

wszdexdrf · 2022-06-23T15:43:44Z

Description

This changes get_balance() function so that it returns balance separated in 4 categories:

available
trusted-pending
untrusted-pending
immature

Fixes#238

Notes to the reviewers

Based on #614

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 updated tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

wszdexdrf · 2022-06-23T15:46:23Z

I have not added tests since I feel that existing test should be fine. I can add some if need be.
Regarding the code, I went with the rule "add instead of change"

danielabrozzoni

Concept ACK

Some tests are failing, run them with cargo test --features test-electrum and cargo test --features test-rpc
You should update the CHANGELOG.md
You should add tests! Some ideas:
- Receive a transaction, check that the output values get counted in the untrusted_pending, get it confirmed, check that now the amount is in available
- Make a transaction, check that the change value is counted in trusted_pending, get it confirmed, check that now the amount is in available

danielabrozzoni · 2022-06-28T08:29:05Z

Can you rebase to pick up the CI fixes (#638)? Thanks :)

evanlinjin

Concept ACK

I'm very excited for these changes, and great work!

I just have some thoughts on naming conventions and some changes in logic. Haven't really gone through everything yet though.

In regards to naming conventions, may need various second opinions.

evanlinjin · 2022-07-05T06:22:59Z

+#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Default)]
+pub struct Balance {
+ /// All coinbase outputs not yet matured
+ pub immature: u64,


Would immature_coinbase be clearer? Or too wordy? 😅

I feel immature coins are only ever meant in regards to coinbase coins, so I think it would be a bit too wordy. I thought of these names as if they had a imaginary "balance" or "coins" added at the end of them.

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?
Not at the moment! But I should open an issue to remember this. Also see #640 (comment)

......which means that maybe immature coinbase is clearer...

afilini · 2022-07-05T20:35:46Z

FYI: #614 got merged, so you can rebase on master to pickup the latest changes.

It also looks like there are some conflicts, so during the rebase you'll get a chance to fix them as well.

EDIT: I think the conflicts mostly come from the two commits you have in your branch from the old version of #614, so just dropping those and rebasing on master should fix everything.

danielabrozzoni

tACK 74def0a

afilini

I hate to make you go through another round of fixing and rebasing but I swear, this is gonna be the last one!

The code looked great to me now, no complains there.

afilini

ACK 62e6b04

evanlinjin

ACK 62e6b04

However, just a thought...

For tests that require checking balance, would it make sense to compare with the whole Balance struct?

For example:

assert_eq!(wallet.get_balance().unwrap(),Balance{ .. })

But I hate to be the one that delays merging, maybe we can have this as another task for another PR?

Otherwise, I've gone through everything and it all looks dandy to me :)

danielabrozzoni · 2022-07-20T13:57:55Z

This needs rebasing to fix the conflicts.

One small note: please, when you've updated the code, can you click "resolve conversation" under the comments? This way the whole PR is way more readable. I've done it for you this time, I hope I didn't accidentally solve something that wasn't solved...

danielabrozzoni

Just a tiny nit. Me and Alekos have been recently talking about lacking test coverage (mainly after we discovered #666 😈 ), and we realize that we should definitely be more nit-picker on tests. Sorry to delay merging even more, but I promise we're close!

Add type balance with add, display traits. Change affected tests. Update `CHANGELOG.md`

danielabrozzoni · 2022-08-04T08:39:38Z

Rebased

afilini

ACK 0f03831

3c6075a Add Balance struct and conversion from BdkBalance (thunderbiscuit) 4e15bad Update BDK to version 0.22 (thunderbiscuit) Pull request description: The bindings do not build when attempting this upgrade because `get_balance()` now returns a `Balance` struct (this was merged in bitcoindevkit/bdk#640) ```sh error[E0308]: mismatched types --> src/lib.rs:433:9 | 432 | fn get_balance(&self) -> Result<u64, Error> { | ------------------ expected `Result<u64, bdk::Error>` because of return type 433 | self.get_wallet().get_balance() | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u64`, found struct `Balance` | = note: expected enum `Result<u64, _>` found enum `Result<Balance, _>` For more information about this error, try `rustc --explain E0308`. error: could not compile `bdk-ffi` due to previous error ``` When we upgrade to `0.22.0` we could decide to add the `Balance` struct to the bindings, or simply return the total by calling `get_total()`, which returns a `u64` (same as we have now). ACKs for top commit: notmandatory: ACK 3c6075a Tree-SHA512: 13d2f83f992735f4f9619ae339d7834df08385129edf06bac830c298b433571af3f211e92a6da1f4f9646dec27dbd2c6133a035f26eac8757b7a1c94b54b463d

wszdexdrf · 2022-06-23T15:43:44Z

Description

This changes get_balance() function so that it returns balance separated in 4 categories:

available
trusted-pending
untrusted-pending
immature

Fixes#238

Notes to the reviewers

Based on #614

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 updated tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

wszdexdrf · 2022-06-23T15:46:23Z

I have not added tests since I feel that existing test should be fine. I can add some if need be.
Regarding the code, I went with the rule "add instead of change"

danielabrozzoni

Concept ACK

Some tests are failing, run them with cargo test --features test-electrum and cargo test --features test-rpc
You should update the CHANGELOG.md
You should add tests! Some ideas:
- Receive a transaction, check that the output values get counted in the untrusted_pending, get it confirmed, check that now the amount is in available
- Make a transaction, check that the change value is counted in trusted_pending, get it confirmed, check that now the amount is in available

danielabrozzoni · 2022-06-28T08:29:05Z

Can you rebase to pick up the CI fixes (#638)? Thanks :)

evanlinjin

Concept ACK

I'm very excited for these changes, and great work!

I just have some thoughts on naming conventions and some changes in logic. Haven't really gone through everything yet though.

In regards to naming conventions, may need various second opinions.

evanlinjin · 2022-07-05T06:22:59Z

+#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Default)]
+pub struct Balance {
+ /// All coinbase outputs not yet matured
+ pub immature: u64,


Would immature_coinbase be clearer? Or too wordy? 😅

I feel immature coins are only ever meant in regards to coinbase coins, so I think it would be a bit too wordy. I thought of these names as if they had a imaginary "balance" or "coins" added at the end of them.

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?
Not at the moment! But I should open an issue to remember this. Also see #640 (comment)

......which means that maybe immature coinbase is clearer...

afilini · 2022-07-05T20:35:46Z

FYI: #614 got merged, so you can rebase on master to pickup the latest changes.

It also looks like there are some conflicts, so during the rebase you'll get a chance to fix them as well.

EDIT: I think the conflicts mostly come from the two commits you have in your branch from the old version of #614, so just dropping those and rebasing on master should fix everything.

danielabrozzoni

tACK 74def0a

afilini

I hate to make you go through another round of fixing and rebasing but I swear, this is gonna be the last one!

The code looked great to me now, no complains there.

afilini

ACK 62e6b04

evanlinjin

ACK 62e6b04

However, just a thought...

For tests that require checking balance, would it make sense to compare with the whole Balance struct?

For example:

assert_eq!(wallet.get_balance().unwrap(),Balance{ .. })

But I hate to be the one that delays merging, maybe we can have this as another task for another PR?

Otherwise, I've gone through everything and it all looks dandy to me :)

danielabrozzoni · 2022-07-20T13:57:55Z

This needs rebasing to fix the conflicts.

One small note: please, when you've updated the code, can you click "resolve conversation" under the comments? This way the whole PR is way more readable. I've done it for you this time, I hope I didn't accidentally solve something that wasn't solved...

danielabrozzoni

Just a tiny nit. Me and Alekos have been recently talking about lacking test coverage (mainly after we discovered #666 😈 ), and we realize that we should definitely be more nit-picker on tests. Sorry to delay merging even more, but I promise we're close!

Add type balance with add, display traits. Change affected tests. Update `CHANGELOG.md`

danielabrozzoni · 2022-08-04T08:39:38Z

Rebased

afilini

ACK 0f03831

3c6075a Add Balance struct and conversion from BdkBalance (thunderbiscuit) 4e15bad Update BDK to version 0.22 (thunderbiscuit) Pull request description: The bindings do not build when attempting this upgrade because `get_balance()` now returns a `Balance` struct (this was merged in bitcoindevkit/bdk#640) ```sh error[E0308]: mismatched types --> src/lib.rs:433:9 | 432 | fn get_balance(&self) -> Result<u64, Error> { | ------------------ expected `Result<u64, bdk::Error>` because of return type 433 | self.get_wallet().get_balance() | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u64`, found struct `Balance` | = note: expected enum `Result<u64, _>` found enum `Result<Balance, _>` For more information about this error, try `rustc --explain E0308`. error: could not compile `bdk-ffi` due to previous error ``` When we upgrade to `0.22.0` we could decide to add the `Balance` struct to the bindings, or simply return the total by calling `get_total()`, which returns a `u64` (same as we have now). ACKs for top commit: notmandatory: ACK 3c6075a Tree-SHA512: 13d2f83f992735f4f9619ae339d7834df08385129edf06bac830c298b433571af3f211e92a6da1f4f9646dec27dbd2c6133a035f26eac8757b7a1c94b54b463d

wszdexdrf · 2022-06-23T15:43:44Z

Description

This changes get_balance() function so that it returns balance separated in 4 categories:

available
trusted-pending
untrusted-pending
immature

Fixes#238

Notes to the reviewers

Based on #614

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 updated tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

wszdexdrf · 2022-06-23T15:46:23Z

I have not added tests since I feel that existing test should be fine. I can add some if need be.
Regarding the code, I went with the rule "add instead of change"

danielabrozzoni

Concept ACK

Some tests are failing, run them with cargo test --features test-electrum and cargo test --features test-rpc
You should update the CHANGELOG.md
You should add tests! Some ideas:
- Receive a transaction, check that the output values get counted in the untrusted_pending, get it confirmed, check that now the amount is in available
- Make a transaction, check that the change value is counted in trusted_pending, get it confirmed, check that now the amount is in available

danielabrozzoni · 2022-06-28T08:29:05Z

Can you rebase to pick up the CI fixes (#638)? Thanks :)

evanlinjin

Concept ACK

I'm very excited for these changes, and great work!

I just have some thoughts on naming conventions and some changes in logic. Haven't really gone through everything yet though.

In regards to naming conventions, may need various second opinions.

evanlinjin · 2022-07-05T06:22:59Z

+#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Default)]
+pub struct Balance {
+ /// All coinbase outputs not yet matured
+ pub immature: u64,


Would immature_coinbase be clearer? Or too wordy? 😅

I feel immature coins are only ever meant in regards to coinbase coins, so I think it would be a bit too wordy. I thought of these names as if they had a imaginary "balance" or "coins" added at the end of them.

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?

That's an interesting point. If a coin is un-spendable due to a script-level timelock (such as OP_CLTV or OP_CSV) would we also use the term immature?
Not at the moment! But I should open an issue to remember this. Also see #640 (comment)

......which means that maybe immature coinbase is clearer...

afilini · 2022-07-05T20:35:46Z

FYI: #614 got merged, so you can rebase on master to pickup the latest changes.

It also looks like there are some conflicts, so during the rebase you'll get a chance to fix them as well.

EDIT: I think the conflicts mostly come from the two commits you have in your branch from the old version of #614, so just dropping those and rebasing on master should fix everything.

danielabrozzoni

tACK 74def0a

afilini

I hate to make you go through another round of fixing and rebasing but I swear, this is gonna be the last one!

The code looked great to me now, no complains there.

afilini

ACK 62e6b04

evanlinjin

ACK 62e6b04

However, just a thought...

For tests that require checking balance, would it make sense to compare with the whole Balance struct?

For example:

assert_eq!(wallet.get_balance().unwrap(),Balance{ .. })

But I hate to be the one that delays merging, maybe we can have this as another task for another PR?

Otherwise, I've gone through everything and it all looks dandy to me :)

danielabrozzoni · 2022-07-20T13:57:55Z

This needs rebasing to fix the conflicts.

One small note: please, when you've updated the code, can you click "resolve conversation" under the comments? This way the whole PR is way more readable. I've done it for you this time, I hope I didn't accidentally solve something that wasn't solved...

danielabrozzoni

Just a tiny nit. Me and Alekos have been recently talking about lacking test coverage (mainly after we discovered #666 😈 ), and we realize that we should definitely be more nit-picker on tests. Sorry to delay merging even more, but I promise we're close!

Add type balance with add, display traits. Change affected tests. Update `CHANGELOG.md`

danielabrozzoni · 2022-08-04T08:39:38Z

Rebased

afilini

ACK 0f03831

3c6075a Add Balance struct and conversion from BdkBalance (thunderbiscuit) 4e15bad Update BDK to version 0.22 (thunderbiscuit) Pull request description: The bindings do not build when attempting this upgrade because `get_balance()` now returns a `Balance` struct (this was merged in bitcoindevkit/bdk#640) ```sh error[E0308]: mismatched types --> src/lib.rs:433:9 | 432 | fn get_balance(&self) -> Result<u64, Error> { | ------------------ expected `Result<u64, bdk::Error>` because of return type 433 | self.get_wallet().get_balance() | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u64`, found struct `Balance` | = note: expected enum `Result<u64, _>` found enum `Result<Balance, _>` For more information about this error, try `rustc --explain E0308`. error: could not compile `bdk-ffi` due to previous error ``` When we upgrade to `0.22.0` we could decide to add the `Balance` struct to the bindings, or simply return the total by calling `get_total()`, which returns a `u64` (same as we have now). ACKs for top commit: notmandatory: ACK 3c6075a Tree-SHA512: 13d2f83f992735f4f9619ae339d7834df08385129edf06bac830c298b433571af3f211e92a6da1f4f9646dec27dbd2c6133a035f26eac8757b7a1c94b54b463d

wszdexdrf force-pushed the get_balance_in_categories branch from f04341b to 13e9ce5Compare June 23, 2022 16:04

wszdexdrf marked this pull request as ready for review June 23, 2022 16:12

danielabrozzoni suggested changes Jun 24, 2022
View reviewed changes

Comment threadsrc/types.rs Outdated
Comment threadsrc/types.rs Outdated
Comment threadsrc/types.rs
Comment threadsrc/wallet/mod.rs Outdated
Comment threadsrc/wallet/mod.rs Outdated
Comment threadsrc/wallet/mod.rs Outdated
Comment threadsrc/wallet/mod.rs

wszdexdrf force-pushed the get_balance_in_categories branch 3 times, most recently from 7e7d25a to ae5271aCompare June 25, 2022 15:06

wszdexdrf force-pushed the get_balance_in_categories branch 5 times, most recently from d91b8fa to 4f052feCompare June 30, 2022 13:45

afilini reviewed Jul 1, 2022
View reviewed changes

Comment threadsrc/wallet/mod.rs Outdated
Comment threadsrc/types.rs Outdated
Comment threadsrc/types.rs Outdated
Comment threadsrc/types.rs
Comment threadsrc/types.rs
Comment threadsrc/wallet/mod.rs

wszdexdrf force-pushed the get_balance_in_categories branch from 4f052fe to ac23fa3Compare July 2, 2022 09:19

notmandatory assigned wszdexdrf Jul 4, 2022

notmandatory added the new feature New feature or request label Jul 4, 2022

evanlinjin requested changes Jul 5, 2022
View reviewed changes

wszdexdrf force-pushed the get_balance_in_categories branch 4 times, most recently from 4ec63e4 to 74def0aCompare July 6, 2022 10:30

danielabrozzoni approved these changes Jul 6, 2022
View reviewed changes

Comment threadsrc/wallet/mod.rs Outdated

wszdexdrf force-pushed the get_balance_in_categories branch from 74def0a to 779a5d3Compare July 6, 2022 13:36

afilini reviewed Jul 6, 2022
View reviewed changes

Comment threadCHANGELOG.md Outdated

wszdexdrf force-pushed the get_balance_in_categories branch 2 times, most recently from bde59df to 62e6b04Compare July 6, 2022 15:21

afilini approved these changes Jul 6, 2022
View reviewed changes

evanlinjin approved these changes Jul 6, 2022
View reviewed changes

wszdexdrf force-pushed the get_balance_in_categories branch from 62e6b04 to 9c1d769Compare July 7, 2022 04:42

danielabrozzoni mentioned this pull request Apr 7, 2025
Maybe we should mark which coins are confirmed but not yet spendable in the balance? bitcoindevkit/bdk_wallet#180
Open

danielabrozzoni added this to the Release 0.21.0 Feature Freeze milestone Jul 20, 2022

wszdexdrf force-pushed the get_balance_in_categories branch 3 times, most recently from 8c626d4 to 2c20ecdCompare July 23, 2022 08:21

danielabrozzoni reviewed Jul 27, 2022
View reviewed changes

Comment threadsrc/wallet/mod.rs Outdated

wszdexdrf force-pushed the get_balance_in_categories branch from 2c20ecd to b9a5e5bCompare July 27, 2022 12:03

danielabrozzoni reviewed Jul 27, 2022
View reviewed changes

Comment threadsrc/testutils/blockchain_tests.rs Outdated
Comment threadsrc/testutils/blockchain_tests.rs Outdated

wszdexdrf force-pushed the get_balance_in_categories branch 2 times, most recently from f3bb345 to b86b3aaCompare July 31, 2022 11:29

danielabrozzoni mentioned this pull request Aug 1, 2022
W31 BDK Library Team Call bitcoindevkit/.github#16
Closed
20 tasks

wszdexdrf force-pushed the get_balance_in_categories branch 7 times, most recently from 34fce23 to fb7845cCompare August 3, 2022 18:42

Change get_balance to return in categories.
0f03831
Add type balance with add, display traits. Change affected tests. Update `CHANGELOG.md`

danielabrozzoni force-pushed the get_balance_in_categories branch from fb7845c to 0f03831Compare August 4, 2022 08:39

afilini approved these changes Aug 4, 2022
View reviewed changes

afilini merged commit 03d3c78 into bitcoindevkit:masterAug 4, 2022

thunderbiscuit mentioned this pull request Sep 1, 2022
Update bdk dependency to 0.22 bitcoindevkit/bdk-ffi#193
Merged

notmandatory mentioned this pull request Sep 23, 2024
misleading spendable balance #797
Closed