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.

Sign up for GitHub

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

Jump to bottom

NODE-2618 Fixed GRPC getNFTList #3904

Merged
merged 8 commits into from
Nov 27, 2023
Merged

NODE-2618 Fixed GRPC getNFTList #3904

merged 8 commits into from
Nov 27, 2023

Conversation

xrtm000
Copy link
Member

@xrtm000 xrtm000 commented Oct 23, 2023

No description provided.

@xrtm000 xrtm000 marked this pull request as ready for review October 23, 2023 12:00
@xrtm000 xrtm000 requested a review from phearnot as a code owner October 23, 2023 12:00
phearnot
phearnot requested changes Nov 6, 2023
View reviewed changes
Copy link
Member

@phearnot phearnot left a comment

Choose a reason for hiding this comment

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

Please add the following test: given the list [N1, N2, N3, N4, N5], limit=2 and after=N2, the method should return [N3, N4]. It should also work across the liquid block boundaries (e.g. when N2 is in the DB, and N3..N5 are in the liquid block).

grpc-server/src/main/scala/com/wavesplatform/api/grpc/AssetsApiGrpcImpl.scala Outdated
@@ -35,7 +35,7 @@ class AssetsApiGrpcImpl(assetsApi: CommonAssetsApi, accountsApi: CommonAccountsA
.concatMapIterable(_.map { case (a, d) =>
NFTResponse(a.id.toByteString, Some(assetInfoResponse(d)))
})
.take(request.limit)
.takeLast(request.limit)
Copy link
Member

Choose a reason for hiding this comment

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

This does not seem right: it would always return the last request.limit entries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@xrtm000 xrtm000 requested a review from phearnot November 7, 2023 08:04
vsuharnikov
vsuharnikov requested changes Nov 9, 2023
View reviewed changes
Copy link
Member

@vsuharnikov vsuharnikov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm ready to discuss this:

node/src/main/scala/com/wavesplatform/api/common/AddressPortfolio.scala Outdated Show resolved Hide resolved
node/src/main/scala/com/wavesplatform/api/common/AddressPortfolio.scala Outdated Show resolved Hide resolved
@phearnot phearnot requested a review from vsuharnikov November 15, 2023 08:17
@phearnot phearnot self-assigned this Nov 27, 2023
@phearnot phearnot force-pushed the node-2618-grpc-nft-list-fix branch from 5a822a7 to 833a7c3 Compare November 27, 2023 11:11
@phearnot phearnot merged commit 9d72e58 into version-1.5.x Nov 27, 2023
1 check passed
@phearnot phearnot deleted the node-2618-grpc-nft-list-fix branch November 27, 2023 12:09
@phearnot phearnot removed their assignment Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DrBlast DrBlast DrBlast approved these changes

@vsuharnikov vsuharnikov vsuharnikov approved these changes

@phearnot phearnot phearnot approved these changes

@ivan-mashonskiy ivan-mashonskiy Awaiting requested review from ivan-mashonskiy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xrtm000 @phearnot @vsuharnikov @DrBlast