[graphql] Add support for clever error resolution in graphql #17338

tzakian · 2024-04-25T18:51:29Z

Description

Adds support for clever errors to GraphQL. So now, if you have code like the following:

module p::m {
   #[error]
    const ENotFound: vector<u8> = b"Element was unable to be found in the vector"

    public fun abort() {
        assert!(false, ENotFound); // Can also be an `abort ENotFound` 
    }
}

This will result in a humand-readable output from graphql:

Error in 1st command
'ENotFound' from module '0x8fd7251015bfd1dc4283bb3d38dc95a2769f75d621c871a81bb9c191a2860aaf::m' in function 'abort' at source line 6
The element was unable to be found in the vector

Test plan

Added tests for all supported constant types, along with tests to make sure that we properly handle package upgrades and resolving to the correct version of a package when resolving clever errors.

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

vercel · 2024-04-25T18:51:36Z

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

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 1, 2024 5:30pm

3 Ignored Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	May 1, 2024 5:30pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	May 1, 2024 5:30pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	May 1, 2024 5:30pm

amnn

Glorious -- the only comment that maybe warrants discussion is the error formatting which we could bikeshed on forever. The important things for me are that we make it more compact than it is today, and that the wording we use for these errors is a recognisable evolution from the default abort code messaging (and so on, as more data is available, the error message remains recognisably part of the same family).

I've left a suggestion for that, but I'll leave it with you -- feel free to re-request review if you think I'm completely off-base and we can discuss.

crates/sui-graphql-e2e-tests/tests/errors/clever_errors.move

crates/sui-graphql-rpc/src/types/transaction_block_effects.rs

crates/sui-package-resolver/src/lib.rs

tzakian · 2024-04-30T23:50:08Z

crates/sui-graphql-e2e-tests/tests/transactions/errors.exp

@@ -20,7 +20,7 @@ Response: {
        {
          "effects": {
            "status": "FAILURE",
-            "errors": "Move Runtime Abort. Location: 83f3be7571dccac8f0fd562cf4aa66707bafe3996c49b6f659be49a9841653fe::m::boom (function index 1) at offset 1, Abort Code: 42 in 1st command."
+            "errors": "Error in 1st command, Move Runtime Abort. Location: 83f3be7571dccac8f0fd562cf4aa66707bafe3996c49b6f659be49a9841653fe::m::boom (function index 1) at offset 1, Abort Code: 42"


@amnn I believe this change is fine since it's just changing the format of a string value, but want to double-check before landing that this change to existing (non-clever) Move aborts is OK?

Yeah it's all good (but this is a good call out on what I meant by my earlier question about error formats -- it's this that I wanted to align with the clever error format, or vice versa).

amnn · 2024-05-01T13:12:35Z

crates/sui-graphql-e2e-tests/tests/errors/clever_errors.exp

+        {
+          "effects": {
+            "status": "FAILURE",
+            "errors": "Error in 1st command, from '0x549be5f8d75cf6ec1eedf3f3e5ce807ccbba27074c87b70f82eb656dcf40edf9::m::callBytevec' (line 53), 'ImNotAString': \u0001\u0002\u0003\u0004\u0005"


nit: your "ImNotAString" example is still being interpreted as a string, but with strange control characters -- I think that's probably okay from an implementation perspective, but means that we should maybe tweak the test case (or add an additional case) for a byte stream that ends up as a Base64 blob.

amnn · 2024-05-01T13:16:01Z

crates/sui-graphql-e2e-tests/tests/errors/clever_errors.exp

+        {
+          "effects": {
+            "status": "FAILURE",
+            "errors": "Error in 1st command, Move Runtime Abort. Location: 549be5f8d75cf6ec1eedf3f3e5ce807ccbba27074c87b70f82eb656dcf40edf9::m::normalAbort (function index 9) at offset 1, Abort Code: 0"


So the symmetrical treatment for the legacy error string would be something like:

Suggested change

"errors": "Error in 1st command, Move Runtime Abort. Location: 549be5f8d75cf6ec1eedf3f3e5ce807ccbba27074c87b70f82eb656dcf40edf9::m::normalAbort (function index 9) at offset 1, Abort Code: 0"

"errors": "Error in 1st command, from '0x549be5f8d75cf6ec1eedf3f3e5ce807ccbba27074c87b70f82eb656dcf40edf9::m::normalAbort' (offset 1), abort code: 0"

Make the location format match the one for clever errors.

(Use the canonical form with a leading 0x).

Drop the function index, and make (offset ...) match (line ...)

fudge the "abort code: X" to look kind of like what we had for the identifier + value for clever errors -- this maybe also implies that we should mention the word "abort" in the other message, to tie the two concepts together? like:

Error in 1st command, from '0x549be5f8d75cf6ec1eedf3f3e5ce807ccbba27074c87b70f82eb656dcf40edf9::m::callString' (line 50), abort 'ImAString': This is a string

amnn · 2024-05-01T13:28:27Z

crates/sui-graphql-rpc/src/types/transaction_block_effects.rs

+                    let ExecutionFailureStatus::MoveAbort(loc, code) = &error else {
+                        break 'error error.to_string();
+                    };
+                    let fname_string = if let Some(fname) = &loc.function_name {
+                        format!("::{}'", fname)
+                    } else {
+                        "'".to_string()
+                    };
+
+                    let Some(CleverError {
+                        module_id,
+                        source_line_number,
+                        error_info,
+                    }) = resolver
+                        .resolve_clever_error(loc.module.clone(), *code)
+                        .await
+                    else {
+                        break 'error error.to_string();
+                    };
+
+                    match error_info {
+                        ConstantErrorInfo::Rendered {
+                            error_identifier,
+                            error_constant,
+                        } => {
+                            format!(
+                                "from '{}{fname_string} (line {source_line_number}), '{error_identifier}': {error_constant}",
+                                module_id.to_canonical_display(true)
+                            )
+                        }
+                        ConstantErrorInfo::Unrendered {
+                            error_identifier,
+                            error_bytes,
+                        } => {
+                            let const_str = FBase64::encode(error_bytes);
+                            format!(
+                                "from '{}{fname_string} (line {source_line_number}), '{error_identifier}': {const_str}",
+                                module_id.to_canonical_display(true)
+                            )
+                        }
+                        ConstantErrorInfo::None => {
+                            format!(
+                                "from '{}{fname_string} (line {source_line_number})",
+                                module_id.to_canonical_display(true)
+                            )
+                        }
+                    }


nit (optional): Might be neater to write! to a string?

let error = 'error: { let ExecutionFailureStatus::MoveAbort(loc, code) = &error else { break 'error error.to_string(); }; let Some(CleverError { module_id, source_line_number, error_info, }) = resolver .resolve_clever_error(loc.module.clone(), *code) .await else { break 'error error.to_string(); }; let mut output = format!("from '{}", module_id.to_canonical_display(true)); let _ = if let Some(fname) = &loc.function_name { write!(output, "::{fname}'") } else { write!(output, "'") } let _ = write!(output, " (line {source_line_number})"); let _ = match error_info { ConstantErrorInfo::Rendered { error_identifier, error_constant, } => write!(output, ", '{error_identifier}': {error_constant}"), ConstantErrorInfo::Unrendered { error_identifier, error_bytes, } => write!(output, ", '{error_identifier}': {}", FBase64::encode(error_bytes)), ConstantErrorInfo::None => { /* nop */ } }; output }

I went to do this, but as I did I found it more confusing (at least to me), so going to keep the current format based way.

amnn · 2024-05-01T13:29:35Z

crates/sui-graphql-rpc/src/types/transaction_block_effects.rs

+    async fn resolve_native_status_impl(
+        &self,
+        resolver: &PackageResolver,
+        status: &mut NativeExecutionStatus,


nit: can we extract this via self as well? I don't think it makes sense for an instance of this type to offer to resolve any status, when that resolution depends on its own PTB commands.

yea, I think we can. Will make this change

amnn · 2024-05-01T13:32:19Z

crates/sui-package-resolver/src/lib.rs

+/// The `ConstantErrorInfo` enum is used to represent the different kinds of error information that
+/// can be returned from a clever error when looking at the constant values for the clever error.
+/// These values are either:
+/// * `None` - No constant information is available, only a line number.
+/// * `Rendered` - The error is a complete error, with an error identifier and constant that can be
+///    rendered in a human-readable format (see in-line doc comments for exact types of values
+///    supported).
+/// * `Unrendered` - If there is an error constant value, but it is not a renderable type (e.g., a
+///   `vector<address>`), then it is treated as opaque and the bytes are returned.


tiny nit (optional): I would shave off some noise in these identifiers:

ConstantErrorInfo -> ErrorConstants

error_identifier -> identifier

error_constant -> constant

error_bytes -> bytes

Unrendered -> Raw

Why waste time say lot word when few word do trick?
-- Kevin

…aphql

…n in graphql

## Description Adds support for clever errors to GraphQL. So now, if you have code like the following: ```move module p::m { #[error] const ENotFound: vector<u8> = b"Element was unable to be found in the vector" public fun abort() { assert!(false, ENotFound); // Can also be an `abort ENotFound` } } ``` This will result in a humand-readable output from graphql: ``` Error in 1st command 'ENotFound' from module '0x8fd7251015bfd1dc4283bb3d38dc95a2769f75d621c871a81bb9c191a2860aaf::m' in function 'abort' at source line 6 The element was unable to be found in the vector ``` ## Test plan Added tests for all supported constant types, along with tests to make sure that we properly handle package upgrades and resolving to the correct version of a package when resolving clever errors. --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [X] GraphQL: Adds support for more understandable and ergonomic Move error codes in Move 2024. - [ ] CLI: - [ ] Rust SDK:

Adds support for clever errors to GraphQL. So now, if you have code like the following: ```move module p::m { #[error] const ENotFound: vector<u8> = b"Element was unable to be found in the vector" public fun abort() { assert!(false, ENotFound); // Can also be an `abort ENotFound` } } ``` This will result in a humand-readable output from graphql: ``` Error in 1st command 'ENotFound' from module '0x8fd7251015bfd1dc4283bb3d38dc95a2769f75d621c871a81bb9c191a2860aaf::m' in function 'abort' at source line 6 The element was unable to be found in the vector ``` Added tests for all supported constant types, along with tests to make sure that we properly handle package upgrades and resolving to the correct version of a package when resolving clever errors. --- Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [X] GraphQL: Adds support for more understandable and ergonomic Move error codes in Move 2024. - [ ] CLI: - [ ] Rust SDK:

…phql (#17338) (#17708) ## Description Adds support for clever errors to GraphQL. So now, if you have code like the following: ```move module p::m { #[error] const ENotFound: vector<u8> = b"Element was unable to be found in the vector" public fun abort() { assert!(false, ENotFound); // Can also be an `abort ENotFound` } } ``` This will result in a humand-readable output from graphql: ``` Error in 1st command 'ENotFound' from module '0x8fd7251015bfd1dc4283bb3d38dc95a2769f75d621c871a81bb9c191a2860aaf::m' in function 'abort' at source line 6 The element was unable to be found in the vector ``` ## Test plan Added tests for all supported constant types, along with tests to make sure that we properly handle package upgrades and resolving to the correct version of a package when resolving clever errors. --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [X] GraphQL: Adds support for more understandable and ergonomic Move error codes in Move 2024. - [ ] CLI: - [ ] Rust SDK:

Adds support for clever errors to GraphQL. So now, if you have code like the following: ```move module p::m { #[error] const ENotFound: vector<u8> = b"Element was unable to be found in the vector" public fun abort() { assert!(false, ENotFound); // Can also be an `abort ENotFound` } } ``` This will result in a humand-readable output from graphql: ``` Error in 1st command 'ENotFound' from module '0x8fd7251015bfd1dc4283bb3d38dc95a2769f75d621c871a81bb9c191a2860aaf::m' in function 'abort' at source line 6 The element was unable to be found in the vector ``` Added tests for all supported constant types, along with tests to make sure that we properly handle package upgrades and resolving to the correct version of a package when resolving clever errors. --- Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [X] GraphQL: Adds support for more understandable and ergonomic Move error codes in Move 2024. - [ ] CLI: - [ ] Rust SDK:

…phql (#17338) (#17711) Adds support for clever errors to GraphQL. So now, if you have code like the following: ```move module p::m { #[error] const ENotFound: vector<u8> = b"Element was unable to be found in the vector" public fun abort() { assert!(false, ENotFound); // Can also be an `abort ENotFound` } } ``` This will result in a humand-readable output from graphql: ``` Error in 1st command 'ENotFound' from module '0x8fd7251015bfd1dc4283bb3d38dc95a2769f75d621c871a81bb9c191a2860aaf::m' in function 'abort' at source line 6 The element was unable to be found in the vector ``` Note that this cherry-pick does not contain the tests as that requires cherry-picking the compiler changes for clever errors, which then involves further cherry-picks as well, so I removed it to keep this as simple as possible.

tzakian requested review from amnn, wlmyng, stefan-mysten, emmazzz and suiwombat as code owners April 25, 2024 18:51

tzakian force-pushed the tzakian/clever-errors-gql branch from 515fb91 to 127ff3d Compare April 25, 2024 23:30

vercel bot deployed to Preview – sui-docs April 25, 2024 23:56 View deployment

amnn approved these changes Apr 27, 2024

View reviewed changes

tzakian force-pushed the tzakian/clever-errors-gql branch from 127ff3d to 9f7d6ae Compare April 30, 2024 23:21

vercel bot deployed to Preview – sui-docs April 30, 2024 23:25 View deployment

tzakian force-pushed the tzakian/clever-errors-gql branch from 9f7d6ae to cdb098a Compare April 30, 2024 23:26

vercel bot deployed to Preview – sui-docs April 30, 2024 23:27 View deployment

tzakian commented Apr 30, 2024

View reviewed changes

vercel bot deployed to Preview – sui-docs April 30, 2024 23:50 View deployment

amnn reviewed May 1, 2024

View reviewed changes

tzakian added 3 commits May 1, 2024 10:23

[graphql] Add support for clever error resolution in graphql

56efc36

fixup! [graphql] Add support for clever error resolution in graphql

9251a6b

fixup! fixup! [graphql] Add support for clever error resolution in gr…

a9f9356

…aphql

tzakian force-pushed the tzakian/clever-errors-gql branch from 79b73f7 to d3b2d1b Compare May 1, 2024 17:27

vercel bot deployed to Preview – sui-docs May 1, 2024 17:28 View deployment

fixup! fixup! fixup! [graphql] Add support for clever error resolutio…

39d6e20

…n in graphql

tzakian force-pushed the tzakian/clever-errors-gql branch from d3b2d1b to 39d6e20 Compare May 1, 2024 17:29

vercel bot deployed to Preview – sui-docs May 1, 2024 17:30 View deployment

tzakian enabled auto-merge (squash) May 1, 2024 17:56

tzakian merged commit 1ced486 into main May 1, 2024
46 of 47 checks passed

tzakian deleted the tzakian/clever-errors-gql branch May 1, 2024 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[graphql] Add support for clever error resolution in graphql #17338

[graphql] Add support for clever error resolution in graphql #17338

tzakian commented Apr 25, 2024

vercel bot commented Apr 25, 2024 •

edited

Loading

amnn left a comment

tzakian Apr 30, 2024

amnn May 1, 2024

amnn May 1, 2024

amnn May 1, 2024

amnn May 1, 2024

tzakian May 1, 2024

amnn May 1, 2024

tzakian May 1, 2024

amnn May 1, 2024

	"errors": "Error in 1st command, Move Runtime Abort. Location: 549be5f8d75cf6ec1eedf3f3e5ce807ccbba27074c87b70f82eb656dcf40edf9::m::normalAbort (function index 9) at offset 1, Abort Code: 0"
	"errors": "Error in 1st command, from '0x549be5f8d75cf6ec1eedf3f3e5ce807ccbba27074c87b70f82eb656dcf40edf9::m::normalAbort' (offset 1), abort code: 0"

[graphql] Add support for clever error resolution in graphql #17338

[graphql] Add support for clever error resolution in graphql #17338

Conversation

tzakian commented Apr 25, 2024

Description

Test plan

Release notes

vercel bot commented Apr 25, 2024 • edited Loading

amnn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Apr 25, 2024 •

edited

Loading