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

[Neo Plugin RPCServer] Rpc parameters. Part I #3457

Merged
merged 21 commits into from
Aug 23, 2024
Merged

[Neo Plugin RPCServer] Rpc parameters. Part I #3457

merged 21 commits into from
Aug 23, 2024

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Aug 11, 2024 •
edited
Loading

Description

Our RPC server methods lacks clear definition on its parameters, thus making it very hard to define its behavior. Thus this pr add a new invoking system that enables us to invoke rpc methods with precise parameters.

Fixes #

Type of change

  • Optimization (the change is only an optimization)
  • Style (the change is only a code style for better maintenance or standard purpose)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Running existing tests.

Test Configuration:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@Jim8y Jim8y changed the title [Neo Plugin RPCServer] Rpc parameters [Neo Plugin RPCServer] Rpc parameters. Part I Aug 11, 2024
@Jim8y
Copy link
Contributor Author

Jim8y commented Aug 11, 2024

This is part one of the system, other apis will be updated in coming prs.

Jim8y
Jim8y commented Aug 11, 2024
View reviewed changes
src/Plugins/RpcServer/RpcServer.cs
methods[name] = method.CreateDelegate<Func<JArray, object>>(handler);
var attribute = method.GetCustomAttribute<RpcMethodAttribute>();
var attributeWithParams = method.GetCustomAttribute<RpcMethodWithParamsAttribute>();
if (attribute is null && attributeWithParams is null) continue;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because i can not do all the convert in one pr, that will make this pr too large to review. thus still keep old attribute. Will be removed in the last pr of this task.

shargon
shargon reviewed Aug 12, 2024
View reviewed changes
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

I love this PR

src/Plugins/RpcServer/RpcServer.cs Outdated Show resolved Hide resolved
src/Plugins/RpcServer/RpcServer.cs Outdated Show resolved Hide resolved
src/Plugins/RpcServer/RpcServer.cs Outdated Show resolved Hide resolved
shargon
shargon reviewed Aug 13, 2024
View reviewed changes
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

Could you fix the nullable warnings with a nullable directive?

@Jim8y
Merge branch 'master' into rpc-parameters
a24573b 
* master:
  Update RpcServer.Blockchain.cs (neo-project#3458)

# Conflicts:
#	src/Plugins/RpcServer/RpcServer.Blockchain.cs
@Jim8y
fix warnings
a1fcf5e
@Jim8y
Copy link
Contributor Author

Jim8y commented Aug 14, 2024

Could you fix the nullable warnings with a nullable directive?

fixed

superboyiii
superboyiii reviewed Aug 14, 2024
View reviewed changes
src/Plugins/RpcServer/RpcServer.Blockchain.cs
{
JToken key = Result.Ok_Or(() => _params[0], RpcError.InvalidParams.WithData($"Invalid Block Hash or Index: {_params[0]}"));
bool verbose = _params.Count >= 2 && _params[1].AsBoolean();
Copy link
Member

@superboyiii superboyiii Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

AsBoolean() can accept 1, but bool can't....
1723618015234

@superboyiii
Copy link
Member

When input number in getblockhash, it seems being converted to double type?
8f31684454c5d2de6756d0e4b085582
48a4231ad2499fee069d04955d6ff86

cschuchardt88
cschuchardt88 reviewed Aug 14, 2024
View reviewed changes
Copy link
Member

@cschuchardt88 cschuchardt88 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.

Why don't you use a real JSON library. I do it for REST server. It auto converts and plugs into kestrel server. Who ever made RPC plugin doesn't know how to use kestrel.

@Jim8y
Copy link
Contributor Author

Jim8y commented Aug 14, 2024 •
edited
Loading

Why don't you use a real JSON library. I do it for REST server. It auto converts and plugs into kestrel server. Who ever made RPC plugin doesn't know how to use kestrel.

Change JSON library is not the purpose of this pr. I dont know why it is using Neo.Json, but this pr will not handle that. Otherwise this pr would be not reviewable cause it will have two major tasks.

We can update to kestrel in the future if possible, step by step, but small steps.

@superboyiii
Copy link
Member

superboyiii commented Aug 15, 2024 •
edited
Loading

Why don't you use a real JSON library. I do it for REST server. It auto converts and plugs into kestrel server. Who ever made RPC plugin doesn't know how to use kestrel.

Neo.Json is more strict than Newton.Json, so if using Newton.Json to create request, somewhere might be incompatible in some cases.

superboyiii
superboyiii approved these changes Aug 16, 2024
View reviewed changes
Copy link
Member

@superboyiii superboyiii left a comment

Choose a reason for hiding this comment

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

Tested OK.

cschuchardt88
cschuchardt88 previously approved these changes Aug 16, 2024
View reviewed changes
@chenzhitong chenzhitong mentioned this pull request Aug 19, 2024
Closed
@NGDAdmin NGDAdmin merged commit 71da68d into neo-project:master Aug 23, 2024
7 checks passed
@Jim8y Jim8y deleted the rpc-parameters branch August 27, 2024 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cschuchardt88 cschuchardt88 cschuchardt88 left review comments

@shargon shargon shargon approved these changes

@AnnaShaleva AnnaShaleva AnnaShaleva approved these changes

@superboyiii superboyiii superboyiii approved these changes

Assignees
No one assigned
Labels
ready to merge waiting for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Jim8y @superboyiii @shargon @cschuchardt88 @AnnaShaleva @NGDAdmin