Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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 field UnconfirmedTransfers to RPC endpoint ListAssets #969

Merged
merged 3 commits into from
Jun 25, 2024

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Jun 24, 2024

Added a new field UnconfirmedTransfers to the ListAssets RPC endpoint response. The field is a count of unconfirmed transfers. This field helps users interpret asset list responses, especially when asset coins are unconfirmed.

This change will hopeful help users understand this sort of situation: #964

@ffranr ffranr self-assigned this Jun 24, 2024
@ffranr ffranr added the gRPC label Jun 24, 2024
@ffranr ffranr requested a review from guggero June 24, 2024 20:04
@ffranr ffranr requested a review from Roasbeef June 24, 2024 20:05
@ffranr ffranr changed the title Add field UnconfirmedTransfersCount to RPC endpoint ListAssets Add field UnconfirmedTransfers to RPC endpoint ListAssets Jun 24, 2024
@ffranr ffranr force-pushed the add-unconfirmedtransferscount-to-listassets branch from 5e9c769 to dd99d6f Compare June 24, 2024 20:23
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very nice, should help a lot with the user experience.
Just need to make sure we only count actually unconfirmed transfers, not all of them.

rpcserver.go Outdated Show resolved Hide resolved
taprpc/taprootassets.proto Outdated Show resolved Hide resolved
@dstadulis
Copy link
Collaborator

Existing issue that might be closed by this PR: #645

Would #969 support reporting pendingchannels in lnd?

@ffranr
Copy link
Contributor Author

ffranr commented Jun 25, 2024

Existing issue that might be closed by this PR: #645

Would #969 support reporting pendingchannels in lnd?

I think pending channels count towards the unconfirmed transfers counter that's being added in this PR.

ffranr added 3 commits June 25, 2024 12:37
Added a new field to the `ListAssets` RPC endpoint response. This field
helps users interpret asset list responses, especially when asset coins
are unconfirmed.
This commit updates an existing test to verify the new
`UnconfirmedTransfers` field in the `ListAssets` RPC endpoint
response.
Add clarification that this endpoint returns both confirmed and
unconfirmed transfers.
@ffranr ffranr force-pushed the add-unconfirmedtransferscount-to-listassets branch from dd99d6f to eeb8a5b Compare June 25, 2024 11:37
@ffranr ffranr requested a review from guggero June 25, 2024 11:38
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice, LGTM 🎉

@ffranr ffranr requested review from gijswijs and removed request for Roasbeef June 25, 2024 13:21
Copy link
Contributor

@gijswijs gijswijs left a comment

Choose a reason for hiding this comment

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

Related to @guggero 's comment earlier (#969 (comment)) I think that the Go Doc comment of QueryParcels is incorrect.

// QueryParcels returns the set of confirmed or unconfirmed parcels.

It should probably read something like this:

// QueryParcels returns the set of confirmed and unconfirmed parcels or exclusively the unconfirmed parcels.

@ffranr
Copy link
Contributor Author

ffranr commented Jun 25, 2024

Related to @guggero 's comment earlier (#969 (comment)) I think that the Go Doc comment of QueryParcels is incorrect.

// QueryParcels returns the set of confirmed or unconfirmed parcels.

It should probably read something like this:

// QueryParcels returns the set of confirmed and unconfirmed parcels or exclusively the unconfirmed parcels.

Yes. But I think I'm using QueryParcels correctly in this PR.

@ffranr ffranr requested a review from gijswijs June 25, 2024 14:35
Copy link
Contributor

@gijswijs gijswijs left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@guggero guggero merged commit b0de73b into main Jun 25, 2024
14 checks passed
@guggero guggero deleted the add-unconfirmedtransferscount-to-listassets branch June 25, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants