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

RPC errors proposal #156

Merged
merged 23 commits into from
Apr 4, 2024
Merged

RPC errors proposal #156

merged 23 commits into from
Apr 4, 2024

Conversation

roman-khimov
Copy link
Contributor

Based on neo-project/neo-modules#728, but differs a bit from it:

  • reduced set of groups
  • unified missing block/header error
  • improved compatibility for -301 and -302

Based on neo-project/neo-modules#728, but differs a bit from it:
 * reduced set of groups
 * unified missing block/header error
 * improved compatibility for -301 and -302
Copy link
Contributor

@ixje ixje left a comment

Choose a reason for hiding this comment

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

I really like supporting this. Many times the error responses are too general to figure out what is actually wrong and one has to debug it running their own node.

nep-Y.mediawiki Outdated Show resolved Hide resolved
nep-Y.mediawiki Outdated Show resolved Hide resolved
nep-Y.mediawiki Outdated Show resolved Hide resolved
nep-Y.mediawiki Outdated Show resolved Hide resolved
nep-Y.mediawiki Outdated Show resolved Hide resolved
nep-Y.mediawiki Outdated Show resolved Hide resolved
nep-Y.mediawiki Outdated Show resolved Hide resolved
nep-Y.mediawiki Outdated Show resolved Hide resolved
nep-Y.mediawiki Outdated Show resolved Hide resolved
roman-khimov and others added 6 commits September 14, 2022 11:14
Co-authored-by: Erik van den Brink <git@erikvandenbrink.com>
Co-authored-by: Erik van den Brink <git@erikvandenbrink.com>
Co-authored-by: Erik van den Brink <git@erikvandenbrink.com>
@roman-khimov
Copy link
Contributor Author

📯 📯 📯 @erikzhang, @shargon, @vncoelho, @devhawk, anyone else 🥁 🥁 🥁

I think we need to move on with this, it's important for dApp development.

shargon
shargon previously approved these changes Dec 12, 2022
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.

Good proposal to improve the user experience

@devhawk
Copy link
Contributor

devhawk commented Jan 18, 2023

Good proposal to improve the user experience

Agreed. When do we merge this?

@steven1227
Copy link
Member

We need this.

These can autoresolve height to hash, so height-related error code
is applicable as well.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Oracle response can have invalid signature.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Blocks contain transactions, therefore any transaction-related
error codes can be applicable too.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Proof verification is relevant only for the nodes that store not only
the latest chain state.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@roman-khimov
Copy link
Contributor Author

Minor adjustments (no new codes) made based on implementation experience from nspcc-dev/neo-go#3063.

tatiana-nspcc added a commit to tatiana-nspcc/neo-go that referenced this pull request Aug 16, 2023
According to proposal:
neo-project/proposals#156

Close nspcc-dev#2248

Signed-off-by: Tatiana Nesterenko <tatiana@nspcc.io>
tatiana-nspcc added a commit to tatiana-nspcc/neo-go that referenced this pull request Aug 16, 2023
According to proposal:
neo-project/proposals#156

Close nspcc-dev#2248

Signed-off-by: Tatiana Nesterenko <tatiana@nspcc.io>
tatiana-nspcc added a commit to tatiana-nspcc/neo-go that referenced this pull request Aug 16, 2023
According to proposal:
neo-project/proposals#156

Close nspcc-dev#2248

Signed-off-by: Tatiana Nesterenko <tatiana@nspcc.io>
tatiana-nspcc added a commit to tatiana-nspcc/neo-go that referenced this pull request Aug 16, 2023
According to proposal:
neo-project/proposals#156

Close nspcc-dev#2248

Signed-off-by: Tatiana Nesterenko <tatiana@nspcc.io>
tatiana-nspcc added a commit to tatiana-nspcc/neo-go that referenced this pull request Aug 16, 2023
According to proposal:
neo-project/proposals#156

Close nspcc-dev#2248

Signed-off-by: Tatiana Nesterenko <tatiana@nspcc.io>
tatiana-nspcc added a commit to tatiana-nspcc/neo-go that referenced this pull request Aug 16, 2023
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 <tatiana@nspcc.io>
tatiana-nspcc added a commit to tatiana-nspcc/neo-go that referenced this pull request Aug 16, 2023
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 <tatiana@nspcc.io>
tatiana-nspcc added a commit to tatiana-nspcc/neo-go that referenced this pull request Aug 16, 2023
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 <tatiana@nspcc.io>
@AnnaShaleva
Copy link
Member

AnnaShaleva commented Aug 18, 2023

Just for the record: we've successfully adopted this standard to the NeoGo RPC server in nspcc-dev/neo-go#3063. It works fine in tests and we'll integrate these new error codes into NeoFS as soon as 0.102.0 is released. There's a couple of issues that will be fixed by the usage of new RPC errors standard.

nep-23.mediawiki Outdated Show resolved Hide resolved
README.mediawiki Outdated Show resolved Hide resolved
shargon
shargon previously approved these changes Feb 8, 2024
README.mediawiki Outdated Show resolved Hide resolved
@roman-khimov
Copy link
Contributor Author

While this hasn't been merged yet, two suggestions related to new attributes:

  • treat NVB violation (not yet valid transaction) as -510 Expired (annotation can be extended)
  • add one more code for Conflicts (when transaction can't be accepted because of conflict), -511/-503 don't really fit for it, -500 can be OK with additional data and yet -513 can be even better for debugging

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Mar 18, 2024
mempool.ErrInsufficientFunds is used when sender doesn't have enough
balance to pay the submitted transaction fees (-511 code according to
neo-project/proposals#156). mempool.ErrConflict is
used when sender is not able to pay the overall transactions fee sum in
the pool (generic -500 error according to the proposal).

This bugfix is kind of breaking change for those users who relied on the
old -511 code previously returning "insufficient funds" error.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
roman-khimov added a commit to nspcc-dev/neo-go that referenced this pull request Mar 18, 2024
This is not the way intended in neo-project/proposals#156.
-511 covers _both_ cases because users hardly can distinguish one from another,
it's just that our mempool implementation has error codes for both..

Signed-off-by: Roman Khimov <roman@nspcc.ru>
@superboyiii
Copy link
Member

I think it's good to go. Adding a implement link will be better.

Signed-off-by: Roman Khimov <roman@nspcc.ru>
Signed-off-by: Roman Khimov <roman@nspcc.ru>
Signed-off-by: Roman Khimov <roman@nspcc.ru>
Signed-off-by: Roman Khimov <roman@nspcc.ru>
@roman-khimov
Copy link
Contributor Author

Updated, added implementation links.

@superboyiii
Copy link
Member

superboyiii commented Apr 3, 2024

@shargon shargon merged commit c60434e into neo-project:master Apr 4, 2024
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.

10 participants