-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add 'include_leased' flag to ListBalances #1119
Conversation
Pull Request Test Coverage Report for Build 10973284227Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice commit structure! Looks pretty good with a couple of minor things that need to be changed though.
a70845a
to
0aa9037
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Looks great, just one more question about the query (and a couple of additional nits, sorry 😅 ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, pending a rebase/squash and some more nits 🎉
This commit adds 'include_leased' flag to `ListBalances` rpc endpoint. With this flag you show balances that include the leased assets, which are otherwise ignored.
Since we now have the same flags for the `ListAssets` and `ListBalances` calls, we add a check that makes sure the sum of all assets returned by `ListAssets` matches the sum returned in `ListBalances`.
With this commit, we add a unit test for the `QueryBalancesByAsset` and `QueryAssetBalancesByGroup` funcs. The test checks that those balance queries return the expected results with all sorts of grouped and leased assets combined, and with the `includeLeased` flag on and off.
ff6b49c
to
fad2631
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm!
very nice commit structure and thorough unit test 💯
@@ -1913,7 +1913,9 @@ func AssertAssetBalances(t *testing.T, client taprpc.TaprootAssetsClient, | |||
|
|||
require.Equal(t, len(allAssets), len(assetIDBalances.AssetBalances)) | |||
|
|||
var totalBalance uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: shouldn't this commit precede the previous itest: test balances with an without leased flag
This PR adds
include_leased
flag to ListBalances rpc endpoint.With this flag you show balances that include the leased assets, which are otherwise ignored. In doing so the behavior is now synced with the
ListAssets
endpoint.Fixes #1106 task 2 (2nd checkbox, and probably also 1st checkbox)
Fixes #1106