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

Handle both number and string for rpc request method #3949

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Odonno
Copy link
Contributor

@Odonno Odonno commented Apr 26, 2024

Thank you for submitting this pull request! We really appreciate you spending the time to work on these changes.

What is the motivation?

When consuming the /rpc endpoint, we have to pass the method name as a string. There is a small optimization to be done by only sending a small number.

What does this change do?

Handle both string and u8 for Method type.

What is your testing strategy?

N/A

Is this related to any issues?

N/A

Does this change need documentation?

Have you read the Contributing Guidelines?

@Odonno Odonno requested review from tobiemh and a team as code owners April 26, 2024 06:52
phughk
phughk previously approved these changes Apr 29, 2024
Copy link
Contributor

@phughk phughk left a comment

Choose a reason for hiding this comment

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

This is really useful thanks so much for this!
LGTM as far as I can see. We might want a test in http_integration.rs for this. Will circle in the team. If you are able to add the test then I think that would be perfect.
Just started the build on it, so assuming its green

@Odonno
Copy link
Contributor Author

Odonno commented Apr 29, 2024

What do you think @phughk?

I could write theory tests as long as a test for each rpc method. Not sure if that would be too much.

@phughk
Copy link
Contributor

phughk commented May 3, 2024

We have had a chat internally with @naisofly and agreed it's ok to merge without a test, but then we will add a test for it once it is merged 👍 Pinging engineering for another tick

@phughk
Copy link
Contributor

phughk commented May 13, 2024

This ticket is waiting on feedback from @tobiemh , as adding it means we have 2 branches of implementation which could lead to unexpected problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants