RPC: add error codes and response errors #3063

tatiana-nspcc · 2023-07-14T17:36:13Z

Problem

We should carefully distinguish all kinds of RPC errors on the server side. Each meaningful error should have a separate meaningful error code. The problem is described in #2248

Solution

Add new error codes and handle them

Ref. neo-project/proposals#156 and https://docs.google.com/spreadsheets/d/1VShwR1qdVeE7J4gGUBzSPnjAIWkhXb-VT9yvf9cau6I/edit#gid=0.

AnnaShaleva

Good. Here are several issues that need to be fixed:

The most significant is that some of errors specified in the proposal remain unused. We have to use all of them, e.g. pay attantion to the contract-based witness verification: errors returned from the blockchain core can be destinguished and RPC server have to distinguish them to return proper (and specific) result from invokecontractverify. Check the other methods as far.
Each commit should contain changes that are grouped by meaning. If some change has a separate meaning or has its own purpose that is not related to the current commit, then create new commit for this change.
Pay attantion to our GH action jobs, the results are attached to the PR. Some of our tests are failing (it's will be fixed in the next release), but the rest of the jobs hsould pass.
Each commit message should include the "signed-off-by" line.
Specify the issue that will be closed by this commit in the commit message: "Close rpc: take care of response errors and error codes #2248".
Commit header should start with a component (or package) name that includes the most significant changes. Use "neorpc: add error codes and response errors" for the current commit.

pkg/neorpc/errors.go

pkg/services/rpcsrv/error.go

pkg/services/rpcsrv/server.go

pkg/services/rpcsrv/server_test.go

pkg/network/server.go

pkg/neorpc/errors.go

roman-khimov · 2023-07-20T14:52:42Z

pkg/neorpc/errors.go

@@ -30,33 +30,95 @@ const (
 const (
 	// RPCErrorCode is returned on RPC request processing error.
 	RPCErrorCode = -100


No longer needed after all of the changes?

No, It's still used in non-specified errors

It shouldn't be, but I guess you have not converted all of them yet.

I get rid of this error code

pkg/neorpc/errors.go

codecov · 2023-07-21T07:03:30Z

Codecov Report

Merging #3063 (90f1b0f) into master (16d1d1e) will increase coverage by 0.12%.
Report is 3 commits behind head on master.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #3063      +/-   ##
==========================================
+ Coverage   84.65%   84.78%   +0.12%     
==========================================
  Files         329      329              
  Lines       43941    43953      +12     
==========================================
+ Hits        37200    37265      +65     
+ Misses       5229     5189      -40     
+ Partials     1512     1499      -13

Files Changed	Coverage Δ
cli/server/server.go	`68.91% <0.00%> (ø)`
pkg/services/rpcsrv/error.go	`90.47% <ø> (-0.83%)`	⬇️
pkg/services/rpcsrv/server.go	`79.82% <74.44%> (+1.90%)`	⬆️
pkg/core/blockchain.go	`78.57% <100.00%> (ø)`
pkg/neorpc/errors.go	`83.87% <100.00%> (+14.17%)`	⬆️

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pkg/neorpc/errors.go

pkg/services/rpcsrv/server.go

AnnaShaleva

It's time to squash all related commits and make the PR structure more clear. I'll review the RPC server part one more time after PR finalization.

pkg/neorpc/errors.go

pkg/network/server.go

tatiana-nspcc

I need some help to get several errors. Anna, could you, please help me?

pkg/services/rpcsrv/server.go

pkg/services/rpcsrv/server_test.go

AnnaShaleva

I haven't reviewd the tests yet, but the overall structure is more clear now.

Note about the first commit: try to overwrite commit message to fit the commit header size limit. The rest of information can be placed into the commit message body.

pkg/core/blockchain.go

pkg/neorpc/errors.go

pkg/services/rpcsrv/server.go

pkg/services/rpcsrv/server_test.go

pkg/core/blockchain.go

pkg/neorpc/errors.go

pkg/services/rpcsrv/server.go

pkg/services/rpcsrv/server_test.go

pkg/core/blockchain.go

pkg/services/rpcsrv/error.go

pkg/services/rpcsrv/server.go

Based on the https://docs.google.com/spreadsheets/d/1VShwR1qdVeE7J4gGUBzSPnjAIWkhXb-VT9yvf9cau6I/edit#gid=0 and nspcc-dev/neo-go#3063. Signed-off-by: Anna Shaleva <[email protected]>

pkg/services/rpcsrv/server.go

pkg/neorpc/errors.go

pkg/core/blockchain.go

pkg/services/rpcsrv/server.go

pkg/services/rpcsrv/client_test.go

pkg/services/rpcsrv/server.go

pkg/services/rpcsrv/server_test.go

roman-khimov

@AnnaShaleva, what do you think about adding

// ErrCompatGeneric is an error returned by nodes not compliant with the neo-project/proposals#156 (NeoGo pre-0.102.0 and all known C# versions). It can be returned for any call and doesn't have any specific meaning.
// Deprecated: to be removed after all nodes adopt new error standard.
ErrCompatGeneric = NewErrorWithCode(-100, "RPC error")
// ErrCompatNoOpenedWallet is an error code returned by nodes not compliant with the neo-project/proposals#156 (all known C# versions, NeoGo never used this code). It can be returned for wallet-related operations.
// Deprecated: to be removed after all nodes adopt new error standard.
ErrCompatGeneric = NewErrorWithCode(-400, "No opened wallet")

The rationale is rather simple, while our server no longer uses these codes they still can come from C# servers and while we consider them deprecated we better at least have some definition of them until C# implements our proposal.

We should also mention the proposal (and the fact that we use a completely different error code set in our server) in docs/rpc.md.

pkg/neorpc/errors.go

pkg/services/rpcsrv/server.go

AnnaShaleva · 2023-08-15T15:25:52Z

@AnnaShaleva, what do you think about adding

// ErrCompatGeneric is an error returned by nodes not compliant with the neo-project/proposals#156 (NeoGo pre-0.102.0 and all known C# versions). It can be returned for any call and doesn't have any specific meaning.
// Deprecated: to be removed after all nodes adopt new error standard.
ErrCompatGeneric = NewErrorWithCode(-100, "RPC error")
// ErrCompatNoOpenedWallet is an error code returned by nodes not compliant with the neo-project/proposals#156 (all known C# versions, NeoGo never used this code). It can be returned for wallet-related operations.
// Deprecated: to be removed after all nodes adopt new error standard.
ErrCompatGeneric = NewErrorWithCode(-400, "No opened wallet")

The rationale is rather simple, while our server no longer uses these codes they still can come from C# servers and while we consider them deprecated we better at least have some definition of them until C# implements our proposal.

Agree, let's do this. @tatiana-nspcc, and then we also need to add a note about these deprecated errors removal to our ROADMAP.md.

Documentation adjustments are needed and minor fixes are required.

roman-khimov

31fe57d and 63dba39 can be squashed as well.

pkg/neorpc/errors.go

docs/rpc.md

pkg/neorpc/errors.go

ROADMAP.md

ErrAlreadyExists is in blockchain and ErrAlreadyInPool is in mempool. Signed-off-by: Tatiana Nesterenko <[email protected]>

AnnaShaleva

Please, fix the links to proposal for all commit messages (use neo-project/proposals#156).

docs/rpc.md

According to proposal: neo-project/proposals#156 Close nspcc-dev#2248 Signed-off-by: Tatiana Nesterenko <[email protected]>

While our server no longer uses these codes (-100, -400) they still can come from C# servers and while we consider them deprecated we better at least have some definition of them until C# implements our proposal: neo-project/proposals#156 Also adding description of deprecated RPC error codes in ROADMAP.md. Signed-off-by: Tatiana Nesterenko <[email protected]>

No functional changes, just a refactoring. Signed-off-by: Tatiana Nesterenko <[email protected]>

Follow the reference implementation, the behaviour was changed in neo-project/neo-modules@237ef7d. Signed-off-by: Tatiana Nesterenko <[email protected]>

No functional changes, just a refactoring. Use more specific and meaningful names to be able to use these errors from external packages. Signed-off-by: Tatiana Nesterenko <[email protected]>

This change makes code incompatible with C# node, because currently no error is returned on invalid proof. According to proposal: neo-project/proposals#156 Also adding `verifyProof` descpiption in docs/rpc.md. Signed-off-by: Tatiana Nesterenko <[email protected]>

Behaviour change. `terminatesession` returns ErrUnknownSession in case of impossibility of finding session, previously there was no-error response with `false` result. `traverseIterator`returns ErrUnknownSession in case of impossibility of finding session, previously there was no-error response with default result; `traverseIterator`returns ErrUnknownIterator, there were no such errors before. Accordingly to proposal: neo-project/proposals#156 Also adding description of `traverseIterator` in docs/rpc.md. Signed-off-by: Tatiana Nesterenko <[email protected]>

No functional changes, just a refactoring. Change error text to be able to use this error from external packages. Signed-off-by: Tatiana Nesterenko <[email protected]>

AnnaShaleva reviewed Jul 17, 2023

View reviewed changes

tatiana-nspcc force-pushed the rpcsrv-errors branch from 1ba7184 to cd0f574 Compare July 17, 2023 13:00

AnnaShaleva reviewed Jul 19, 2023

View reviewed changes

pkg/network/server.go Outdated Show resolved Hide resolved

pkg/neorpc/errors.go Outdated Show resolved Hide resolved

roman-khimov requested changes Jul 20, 2023

View reviewed changes

AnnaShaleva reviewed Jul 24, 2023

View reviewed changes

pkg/neorpc/errors.go Outdated Show resolved Hide resolved

pkg/neorpc/errors.go Outdated Show resolved Hide resolved

pkg/neorpc/errors.go Outdated Show resolved Hide resolved

pkg/services/rpcsrv/server.go Outdated Show resolved Hide resolved

AnnaShaleva mentioned this pull request Jul 25, 2023

Improve Oracle request submission errors #3068

Open

AnnaShaleva reviewed Jul 25, 2023

View reviewed changes

tatiana-nspcc force-pushed the rpcsrv-errors branch from a11cc28 to 7357f19 Compare July 27, 2023 12:20

tatiana-nspcc commented Jul 27, 2023

View reviewed changes

pkg/services/rpcsrv/server.go Show resolved Hide resolved

pkg/services/rpcsrv/server.go Show resolved Hide resolved

pkg/services/rpcsrv/server.go Outdated Show resolved Hide resolved

pkg/services/rpcsrv/server_test.go Show resolved Hide resolved

AnnaShaleva reviewed Jul 28, 2023

View reviewed changes

This was referenced Aug 7, 2023

api: fix wait_for_transaction_receipt for neo-go nodes CityOfZion/neo-mamba#273

Merged

rpcsrv: carefully store Oracle service instance #3085

Merged

AnnaShaleva reviewed Aug 9, 2023

View reviewed changes

tatiana-nspcc force-pushed the rpcsrv-errors branch 4 times, most recently from 907b120 to 0809b6c Compare August 10, 2023 23:41

AnnaShaleva reviewed Aug 11, 2023

View reviewed changes

AnnaShaleva mentioned this pull request Aug 11, 2023

nep-Y: add missing return error codes roman-khimov/proposals#2

Merged

tatiana-nspcc force-pushed the rpcsrv-errors branch from 0809b6c to 88da09c Compare August 13, 2023 21:03

tatiana-nspcc commented Aug 13, 2023

View reviewed changes

pkg/services/rpcsrv/server.go Outdated Show resolved Hide resolved

tatiana-nspcc force-pushed the rpcsrv-errors branch from 88da09c to 409ff47 Compare August 13, 2023 21:47

AnnaShaleva added this to the v0.102.0 milestone Aug 14, 2023

tatiana-nspcc force-pushed the rpcsrv-errors branch from 409ff47 to f606e25 Compare August 14, 2023 09:25

AnnaShaleva reviewed Aug 14, 2023

View reviewed changes

roman-khimov mentioned this pull request Aug 14, 2023

RPC errors proposal neo-project/proposals#156

Merged

AnnaShaleva reviewed Aug 15, 2023

View reviewed changes

pkg/services/rpcsrv/server_test.go Outdated Show resolved Hide resolved

AnnaShaleva requested a review from roman-khimov August 15, 2023 13:31

tatiana-nspcc force-pushed the rpcsrv-errors branch from 355ec06 to 020ff9c Compare August 15, 2023 13:44

AnnaShaleva previously approved these changes Aug 15, 2023

View reviewed changes

roman-khimov reviewed Aug 15, 2023

View reviewed changes

tatiana-nspcc force-pushed the rpcsrv-errors branch from 020ff9c to 587056f Compare August 16, 2023 09:33

tatiana-nspcc requested review from roman-khimov and AnnaShaleva August 16, 2023 09:36

roman-khimov reviewed Aug 16, 2023

View reviewed changes

pkg/neorpc/errors.go Show resolved Hide resolved

AnnaShaleva reviewed Aug 16, 2023

View reviewed changes

docs/rpc.md Show resolved Hide resolved

pkg/neorpc/errors.go Show resolved Hide resolved

ROADMAP.md Outdated Show resolved Hide resolved

core: split ErrAlreadyExists into ErrAlreadyExists and ErrAlreadyInPool

3abddc7

ErrAlreadyExists is in blockchain and ErrAlreadyInPool is in mempool. Signed-off-by: Tatiana Nesterenko <[email protected]>

tatiana-nspcc force-pushed the rpcsrv-errors branch from 587056f to 8581f04 Compare August 16, 2023 11:43

roman-khimov approved these changes Aug 16, 2023

View reviewed changes

AnnaShaleva reviewed Aug 16, 2023

View reviewed changes

docs/rpc.md Outdated Show resolved Hide resolved

docs/rpc.md Outdated Show resolved Hide resolved

docs/rpc.md Outdated Show resolved Hide resolved

tatiana-nspcc added 9 commits August 16, 2023 13:59

neorpc: add error codes and response errors

31ceb56

According to proposal: neo-project/proposals#156 Close nspcc-dev#2248 Signed-off-by: Tatiana Nesterenko <[email protected]>

docs: add proposal regarding response error codes

3178c8f

According to proposal: neo-project/proposals#156 Close nspcc-dev#2248 Signed-off-by: Tatiana Nesterenko <[email protected]>

rpcsrv: remove default request HTTP codes from switch statement

d3fe92d

No functional changes, just a refactoring. Signed-off-by: Tatiana Nesterenko <[email protected]>

rpcsrv: return error on unknown storage item from getstorage

1e6372e

Follow the reference implementation, the behaviour was changed in neo-project/neo-modules@237ef7d. Signed-off-by: Tatiana Nesterenko <[email protected]>

core: rename ErrInvalidVerification and ErrInvalidInvocation

2598257

No functional changes, just a refactoring. Use more specific and meaningful names to be able to use these errors from external packages. Signed-off-by: Tatiana Nesterenko <[email protected]>

core: change text in ErrInvalidVerificationContract

90f1b0f

No functional changes, just a refactoring. Change error text to be able to use this error from external packages. Signed-off-by: Tatiana Nesterenko <[email protected]>

tatiana-nspcc force-pushed the rpcsrv-errors branch from 8581f04 to 90f1b0f Compare August 16, 2023 13:25

tatiana-nspcc requested a review from AnnaShaleva August 16, 2023 13:28

AnnaShaleva approved these changes Aug 16, 2023

View reviewed changes

roman-khimov merged commit 5c6a111 into nspcc-dev:master Aug 16, 2023
13 of 18 checks passed

roman-khimov mentioned this pull request Aug 28, 2023

Implement improved JSON-RPC error code scheme neo-project/neo-modules#814

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC: add error codes and response errors #3063

RPC: add error codes and response errors #3063

tatiana-nspcc commented Jul 14, 2023 •

edited

Loading

AnnaShaleva left a comment •

edited

Loading

roman-khimov Jul 20, 2023

tatiana-nspcc Jul 21, 2023

roman-khimov Jul 21, 2023

tatiana-nspcc Jul 27, 2023

codecov bot commented Jul 21, 2023 •

edited

Loading

AnnaShaleva left a comment

tatiana-nspcc left a comment

AnnaShaleva left a comment

roman-khimov left a comment

AnnaShaleva commented Aug 15, 2023

roman-khimov left a comment

AnnaShaleva left a comment

RPC: add error codes and response errors #3063

RPC: add error codes and response errors #3063

Conversation

tatiana-nspcc commented Jul 14, 2023 • edited Loading

Problem

Solution

AnnaShaleva left a comment • edited Loading

Choose a reason for hiding this comment

roman-khimov Jul 20, 2023

Choose a reason for hiding this comment

tatiana-nspcc Jul 21, 2023

Choose a reason for hiding this comment

roman-khimov Jul 21, 2023

Choose a reason for hiding this comment

tatiana-nspcc Jul 27, 2023

Choose a reason for hiding this comment

codecov bot commented Jul 21, 2023 • edited Loading

Codecov Report

AnnaShaleva left a comment

Choose a reason for hiding this comment

tatiana-nspcc left a comment

Choose a reason for hiding this comment

AnnaShaleva left a comment

Choose a reason for hiding this comment

roman-khimov left a comment

Choose a reason for hiding this comment

AnnaShaleva commented Aug 15, 2023

roman-khimov left a comment

Choose a reason for hiding this comment

AnnaShaleva left a comment

Choose a reason for hiding this comment

tatiana-nspcc commented Jul 14, 2023 •

edited

Loading

AnnaShaleva left a comment •

edited

Loading

codecov bot commented Jul 21, 2023 •

edited

Loading