fix(rpc): always return array reponses for batch requests #3678

omarsy · 2025-02-03T22:03:07Z

closes: #3676

Currently, when a batch request contains only a single item, our implementation returns a single response object instead of an array.

What This PR Does:
My changes update the logic so that even if the batch request includes only one item, the endpoint will still return an array containing that single response.

…uests

Gno2D2 · 2025-02-03T22:04:43Z

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):

IGNORE the bot requirements for this PR (force green CI check)

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)
🟢 Pending initial approval by a review team member, or review from tech-staff

☑️ Contributor Actions:

Fix any issues flagged by automated checks.
Follow the Contributor Checklist to ensure your PR is ready for review.
- Add new tests, or document why they are unnecessary.
- Provide clear examples/screenshots, if necessary.
- Update documentation, if required.
- Ensure no breaking changes, or include BREAKING CHANGE notes.
- Link related issues/PRs, where applicable.

☑️ Reviewer Actions:

Complete manual checks for the PR, including the guidelines and additional checks if applicable.

📚 Resources:

Debug

Automated Checks

Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: omarsy/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Pending initial approval by a review team member, or review from tech-staff

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 Not (🔴 Pull request author is a member of the team: tech-staff)

Then

🟢 Requirement satisfied
└── 🟢 If
    ├── 🟢 Condition
    │   └── 🟢 Or
    │       ├── 🔴 At least 1 user(s) of the organization reviewed the pull request (with state "APPROVED")
    │       ├── 🟢 At least 1 user(s) of the team tech-staff reviewed pull request
    │       └── 🔴 This pull request is a draft
    └── 🟢 Then
        └── 🟢 Not (🔴 This label is applied to pull request: review/triage-pending)

Manual Checks

**IGNORE** the bot requirements for this PR (force green CI check)
If
🟢 Condition met
└── 🟢 On every pull request
Can be checked by

Any user with comment edit permission

codecov · 2025-02-03T22:07:01Z

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
tm2/pkg/bft/rpc/lib/server/http_server.go	42.85%	2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

aeddi

Thanks for this quick PR. 👍
I just tested it locally, and it looks like this script is still failing.

Could you take a look at the comments and let me know if I'm missing something?

aeddi · 2025-02-05T09:09:56Z

tm2/pkg/bft/rpc/lib/server/handlers.go

@@ -145,6 +145,9 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger *slog.Logger) http.H
 			requests  types.RPCRequests
 			responses types.RPCResponses
 		)
+
+		isRPCRequests := true


I don't understand the purpose of this variable. If the point is to know if there is only one request (so only one response) why don't you use a len(requests) instead?
Or event better, a len(responses) in your case (since 1 request == 1 response if I read the for loop bellow correctly).

If there's another purpose, please change the variable name and add a comment to explain it.

Thanks for the review! ^^

In this PR, I've updated the logic for handling batch requests. Previously, if a batch request contained only a single item, the endpoint would return just a single response object instead of an array containing that response. This was inconsistent with the expected behavior where even a single-item batch should yield an array of responses.
With my changes, regardless of the number of items in the batch request, the API now consistently returns an array.

aeddi · 2025-02-05T09:13:47Z

tm2/pkg/bft/rpc/lib/server/handlers.go

+		if len(responses) == 0 {
+			return
+		}
+
+		if isRPCRequests {
 			WriteRPCResponseArrayHTTP(w, responses)
+			return
 		}
+
+		WriteRPCResponseHTTP(w, responses[0])


This code looks like it could be replaced by something like:

if len(responses) == 1 { WriteRPCResponseHTTP(w, responses[0]) } else if if len(responses) > 1 { WriteRPCResponseArrayHTTP(w, responses) }

Or maybe it would better to check this directly inside WriteRPCResponseArrayHTTP function and to just let the code as it is in this file.

I just checked the WriteRPCResponseArrayHTTP and it already check this

gno/tm2/pkg/bft/rpc/lib/server/http_server.go

Lines 122 to 124 in d75f1a2

if len(res) == 1 {

WriteRPCResponseHTTP(w, res[0])

} else {

I'm not sure that these changes in the code don't have any impact then.

:o I forget to commit this 7453add is why it failing sorry ^^

I haven't tried re-running the script with this change because I really feel like this commit won't fix it.
Did you try it on your side? Is it working?

Can you add a unit test that covers the same case as the issue's script please?

Edited the test + add a subtest here: b37dc8c

I haven't tried re-running the script with this change because I really feel like this commit won't fix it. Did you try it on your side? Is it working?

Yes it works

Ok cool :) I'll check it

Ok, it works well, GG 👍

So your fix is not about the number of responses you have to send but whether you received an array or not. If you get an array of size 1, you should return an array of size 1, right?

If that's the case, could you change your boolean name to something like isRPCRequestArray and add a comment right above your last if explaining that for a request in the form of an array, even if it contains only one element, your response must also be an array.

Thanks!

So your fix is not about the number of responses you have to send but whether you received an array or not. If you get an array of size 1, you should return an array of size 1, right?

Yes

If that's the case, could you change your boolean name to something like isRPCRequestArray and add a comment right above your last if explaining that for a request in the form of an array, even if it contains only one element, your response must also be an array.

I used "isRPCRequests" with an added "s", but "isRPCRequestArray" is clearer.
I’ve added a comment here: 50fa2cd.

aeddi

Edit: sorry I spammed the Submit Review button.

fix(rpc): improve JSON-RPC response handling for single and batch req…

a87e0b1

…uests

github-actions bot assigned omarsy Feb 3, 2025

github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Feb 3, 2025

Gno2D2 requested a review from a team February 3, 2025 22:04

omarsy changed the title ~~fix(rpc): improve JSON-RPC response handling for single and batch requesr~~ fix(rpc): improve JSON-RPC response handling for single and batch request Feb 4, 2025

omarsy changed the title ~~fix(rpc): improve JSON-RPC response handling for single and batch request~~ fix(rpc): improve JSON-RPC response handling for single and batch requests Feb 4, 2025

Merge branch 'master' into fix/3676

6380e94

aeddi self-requested a review February 4, 2025 22:32

omarsy marked this pull request as ready for review February 5, 2025 08:44

Gno2D2 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Feb 5, 2025

aeddi requested changes Feb 5, 2025

View reviewed changes

Gno2D2 requested a review from a team February 5, 2025 09:23

Gno2D2 removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Feb 5, 2025

omarsy added 2 commits February 5, 2025 15:43

refactor(rpc): WriteRPCResponseArrayHTTP should return only an array

7453add

add test for the bug

b37dc8c

omarsy changed the title ~~fix(rpc): improve JSON-RPC response handling for single and batch requests~~ fix(rpc): always return array reponses for batch requests Feb 5, 2025

refactor(rpc): rename flag to clarify + add comment

50fa2cd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rpc): always return array reponses for batch requests #3678

fix(rpc): always return array reponses for batch requests #3678

omarsy commented Feb 3, 2025 •

edited

Loading

Gno2D2 commented Feb 3, 2025 •

edited

Loading

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Then

If

Can be checked by

codecov bot commented Feb 3, 2025 •

edited

Loading

aeddi left a comment •

edited

Loading

aeddi Feb 5, 2025

omarsy Feb 5, 2025

aeddi Feb 5, 2025

aeddi Feb 5, 2025

omarsy Feb 5, 2025

aeddi Feb 5, 2025 •

edited

Loading

aeddi Feb 5, 2025

omarsy Feb 5, 2025

omarsy Feb 5, 2025

aeddi Feb 5, 2025

aeddi Feb 5, 2025

omarsy Feb 5, 2025

aeddi left a comment •

edited

Loading

fix(rpc): always return array reponses for batch requests #3678

Are you sure you want to change the base?

fix(rpc): always return array reponses for batch requests #3678

Conversation

omarsy commented Feb 3, 2025 • edited Loading

Gno2D2 commented Feb 3, 2025 • edited Loading

🛠 PR Checks Summary

Manual Checks (for Reviewers):

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Then

If

Can be checked by

codecov bot commented Feb 3, 2025 • edited Loading

Codecov Report

aeddi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeddi Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeddi left a comment • edited Loading

Choose a reason for hiding this comment

omarsy commented Feb 3, 2025 •

edited

Loading

Gno2D2 commented Feb 3, 2025 •

edited

Loading

codecov bot commented Feb 3, 2025 •

edited

Loading

aeddi left a comment •

edited

Loading

aeddi Feb 5, 2025 •

edited

Loading

aeddi left a comment •

edited

Loading