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][structured-error-handling][2/n] State trait for AuthorityState, group AuthorityState-originating SuiErrors #12933

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

wlmyng
Copy link
Contributor

@wlmyng wlmyng commented Jul 10, 2023

Description

Part of the first set of PRs is to group the errors that are possible on the RPC today into a few categories transparently, setting us up to eventually standardize error strings and resolution. This PR specifically introduces a State trait which AuthorityState implements to centralize AuthorityState-originating SuiErrors so that we can disambiguate these errors from other sources of SuiError. This has the added benefit of helping w/ unit test writing, as demonstrated by coin_api and move_utils.

No change has been made to error strings, but StateReadError::Internal will now code to -32603 instead of -32000. But this can be negotiable too.

Test Plan

Existing tests


If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

  • protocol change
  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

Error code designation is updated to support a more cohesive error reporting structure. Internal errors that arise while reading from authority return a -32603 error code. Client-fault errors that arise while reading from authority return a -32602 error code. Error strings are not modified.

@vercel
Copy link

vercel bot commented Jul 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mysten-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 4, 2023 6:52pm
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 4, 2023 6:52pm
6 Ignored Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Visit Preview Aug 4, 2023 6:52pm
explorer-storybook ⬜️ Ignored (Inspect) Aug 4, 2023 6:52pm
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Aug 4, 2023 6:52pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Aug 4, 2023 6:52pm
sui-wallet-kit ⬜️ Ignored (Inspect) Visit Preview Aug 4, 2023 6:52pm
wallet-adapter ⬜️ Ignored (Inspect) Aug 4, 2023 6:52pm

crates/sui-json-rpc/src/coin_api.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/coin_api.rs Outdated Show resolved Hide resolved
@wlmyng wlmyng changed the title Introduce rpc-level trait object for AuthorityState, and finalize AuthorityState-specific error handling [rpc][structured-error-handling][2/4] group and centralize AuthorityState-originating SuiErrors Jul 17, 2023
@wlmyng wlmyng changed the title [rpc][structured-error-handling][2/4] group and centralize AuthorityState-originating SuiErrors [rpc][structured-error-handling][2/n] group and centralize AuthorityState-originating SuiErrors Jul 17, 2023
@wlmyng wlmyng changed the title [rpc][structured-error-handling][2/n] group and centralize AuthorityState-originating SuiErrors [rpc][structured-error-handling][2/n] State trait for AuthorityState, group AuthorityState-originating SuiErrors Jul 19, 2023
crates/sui-json-rpc/src/authority_state.rs Outdated Show resolved Hide resolved
crates/sui-json-rpc/src/authority_state.rs Outdated Show resolved Hide resolved
| SuiError::UnsupportedFeatureError { .. }
| SuiError::UserInputError { .. }
| SuiError::WrongMessageVersion { .. } => StateReadError::Client(e.into()),
_ => StateReadError::Internal(e.into()),
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be useful to clarify somehow when the error is transient, indicating to the user that they can retry, vs an error that is non-transient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah good call!

crates/sui-json-rpc/src/authority_state.rs Outdated Show resolved Hide resolved
@lavindir
Copy link
Contributor

Approved with the above suggestions applied

…tyState as StateReadError. This will help us later when handling QuorumDriverErrror and SuiErrors that may arise from other situations
@wlmyng wlmyng merged commit 72d2fb7 into main Aug 9, 2023
38 checks passed
@wlmyng wlmyng deleted the dyn-state-trait-for-rpc branch August 9, 2023 21:52
ebmifa pushed a commit that referenced this pull request Aug 10, 2023
… group AuthorityState-originating SuiErrors (#12933)

## Description 

Part of the first set of PRs is to group the errors that are possible on
the RPC today into a few categories transparently, setting us up to
eventually standardize error strings and resolution. This PR
specifically introduces a `State` trait which `AuthorityState`
implements to centralize `AuthorityState`-originating `SuiError`s so
that we can disambiguate these errors from other sources of `SuiError`.
This has the added benefit of helping w/ unit test writing, as
demonstrated by `coin_api` and `move_utils`.

No change has been made to error strings, but StateReadError::Internal
will now code to `-32603` instead of `-32000`. But this can be
negotiable too.

## Test Plan 

Existing tests

---
If your changes are not user-facing and not a breaking change, you can
skip the following section. Otherwise, please indicate what changed, and
then add to the Release Notes section as highlighted during the release
process.

### Type of Change (Check all that apply)

- [ ] protocol change
- [x] user-visible impact
- [x] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes
Internal errors that arise while reading from authority will now return
-32603 error code. Client-fault errors that arise while reading from
authority will now return -32602 error code. Error strings have not been
modified.
ebmifa added a commit that referenced this pull request Aug 29, 2023
## Description 
Adding protocol change highlight to release notes script

## Test Plan 
```
eugene@mac-studio ~/code/sui (eugene/add_protocol_highlight_to_release_notes) $ ./scripts/generate-release-notes.sh releases/sui-v1.7.0-release releases/sui-v1.8.0-release
Sui Protocol Version in this release: XX


#13124:
Add protocol config feature flags for zkLogin to enable testing in Devnet, use updated proof verification logics for zkLogin signature verification.

#13417:

When building Move code, there are now additional linter warnings related to comparing collections from Sui framework code (`Bag`, `Table`, and `TableVec`). Note that this comparison is not a structural one based on the collection content, which is what one might expect, so Sui now indicates this via a linter warning.

#12989:
All transaction execution errors from `execute_transaction_block` of `client-fault` now return a -32002 error code. If you encounter this error code, there is most likely an issue in your transaction inputs.
Previously, when executing a transaction failed on the RPC, you would receive a, "Transaction has non recoverable errors from at least 1/3 of validators" after the transaction failed to execute. You now receive an improved error message, "Transaction execution failed due to issues with transaction inputs, please review the errors and try again: {errors}", where `{errors}` is a string list of actionable errors. After you resolve the errors indicated, your transaction should succeed.

#13194:

When building Move code, there are now additional linter warnings related to freezing an object containing (directly or indirectly) other (wrapped) object. Freezing such an object prevents unwrapping of inner objects.

#12575:

The details included in error messages returned during dependency graph construction might differ from the previous error messages, but they still include similar details and information.


#12933:
Error code designation is updated to support a more cohesive error reporting structure. Internal errors that arise while reading from authority return a `-32603` error code. Client-fault errors that arise while reading from authority return a `-32602` error code. Error strings are not modified.

#13312:

Removes the `--legacy-digest` flag from the `sui client upgrade` and `sui move build` CLI commands, as Sui networks no longer require package digests to be calculated using the legacy algorithm.
```
randall-Mysten pushed a commit that referenced this pull request Sep 6, 2023
## Description 
Adding protocol change highlight to release notes script

## Test Plan 
```
eugene@mac-studio ~/code/sui (eugene/add_protocol_highlight_to_release_notes) $ ./scripts/generate-release-notes.sh releases/sui-v1.7.0-release releases/sui-v1.8.0-release
Sui Protocol Version in this release: XX


#13124:
Add protocol config feature flags for zkLogin to enable testing in Devnet, use updated proof verification logics for zkLogin signature verification.

#13417:

When building Move code, there are now additional linter warnings related to comparing collections from Sui framework code (`Bag`, `Table`, and `TableVec`). Note that this comparison is not a structural one based on the collection content, which is what one might expect, so Sui now indicates this via a linter warning.

#12989:
All transaction execution errors from `execute_transaction_block` of `client-fault` now return a -32002 error code. If you encounter this error code, there is most likely an issue in your transaction inputs.
Previously, when executing a transaction failed on the RPC, you would receive a, "Transaction has non recoverable errors from at least 1/3 of validators" after the transaction failed to execute. You now receive an improved error message, "Transaction execution failed due to issues with transaction inputs, please review the errors and try again: {errors}", where `{errors}` is a string list of actionable errors. After you resolve the errors indicated, your transaction should succeed.

#13194:

When building Move code, there are now additional linter warnings related to freezing an object containing (directly or indirectly) other (wrapped) object. Freezing such an object prevents unwrapping of inner objects.

#12575:

The details included in error messages returned during dependency graph construction might differ from the previous error messages, but they still include similar details and information.


#12933:
Error code designation is updated to support a more cohesive error reporting structure. Internal errors that arise while reading from authority return a `-32603` error code. Client-fault errors that arise while reading from authority return a `-32602` error code. Error strings are not modified.

#13312:

Removes the `--legacy-digest` flag from the `sui client upgrade` and `sui move build` CLI commands, as Sui networks no longer require package digests to be calculated using the legacy algorithm.
```
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

4 participants