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

[CLI] Add clever error support to Sui CLI #17920

Merged
merged 5 commits into from
Jun 19, 2024
Merged

Conversation

tzakian
Copy link
Contributor

@tzakian tzakian commented May 24, 2024

Description

Enables clever error rendering in the Sui CLI.

It also abstracts out some of the rendering logic for constants into their own file so it can be reused across the graphql and Sui CLI renderer.

This also updates the Move disassembler to use this new (nicer!) renderer to that we support rendered values for constants in the Move disassembler instead of just strings in constants.

Test plan

Added new tests for this to the Sui CLI, and updated/added additional tests for the disassembler.


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:
  • GraphQL:
  • CLI: Add better rendering for Move aborts, and added support for rendering clever errors.
  • Rust SDK:

@tzakian tzakian requested review from amnn, awelc and stefan-mysten May 24, 2024 18:59
Copy link

vercel bot commented May 24, 2024

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 Jun 18, 2024 11:45pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Jun 18, 2024 11:45pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Jun 18, 2024 11:45pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Jun 18, 2024 11:45pm

@tzakian tzakian requested review from a team and removed request for awelc May 24, 2024 18:59
Copy link
Contributor

@stefan-mysten stefan-mysten left a comment

Choose a reason for hiding this comment

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

For the CLI part, looks good. Excited to see this rolled out!

I am curious about what are your thoughts on having these available also for dry-run?

crates/sui/src/clever_error_rendering.rs Outdated Show resolved Hide resolved
crates/sui/src/clever_error_rendering.rs Outdated Show resolved Hide resolved
---
Non-clever-abort
---
Error executing transaction: 1st command aborted within function '0xc9d9682e45a4291cf2c7d86486f604fe19451bb367cc9fa4f2149eb5f1efac8a::clever_errors::aborter' at instruction 1 with code 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have an address map or anything at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe we do. But I'll look around to see -- I could look in the CLI address aliases and possibly look about using them if an address matches.

Copy link
Contributor

@stefan-mysten stefan-mysten May 30, 2024

Choose a reason for hiding this comment

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

Pardon my noob question, but isn't this a package address? That would not exist in the CLI aliases' file, because it only hosts wallet's addresses and aliases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😢 Nope, you're totally right. Nothing we can really do about it then (at least right now).

Copy link
Member

Choose a reason for hiding this comment

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

An idea I had a long time ago was to look for a Move.toml and use the address <-> name mapping that it defines for this purpose (I wanted to do this to improve our representation of PTBs in the CLI, with the idea that you could start building up a Move.toml file that depended on other packages and/or assigned names to addresses as you made more sense of a PTB).

@tzakian
Copy link
Contributor Author

tzakian commented Jun 13, 2024

This should be updated so we now render clever errors in the displays as well. I had...a lot of fun with the fact that Display is not async in Rust before coming to the pre-rendering solution here.

Copy link
Member

@amnn amnn left a comment

Choose a reason for hiding this comment

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

Nice -- accepting with comments assuming that on the CLI side the majority of those changes are to tide us over until we have proper GraphQL support, in which case clever_error_rendering.rs can be swept under the rug and we can pretend we never had to resort to such things.

Copy link
Member

Choose a reason for hiding this comment

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

Some doc comments would be nice here.

Copy link
Member

Choose a reason for hiding this comment

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

This code looks suspiciously similar to the way GraphQL renders errors, but using the SDK to fetch package information rather than a package resolver. This yields many questions:

  • Is this only used to render errors coming from on-chain, or is it also used to render local errors, e.g. from move test?
    • If only from on-chain (including localnet), do you envisage this sticking around for a while, or is this a stop-gap until we have a GraphQL-backed CLI?
    • If also for local errors, how does it get the package information for the local package? (I'm guessing it doesn't and so it's only for on-chain).
  • If you do expect this logic to stick around for a while, then should we factor out the common parts with GraphQL?

Context answering these questions would be helpful to include in the aforementioned doc comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to add some doc comments :)

Is this only used to render errors coming from on-chain, or is it also used to render local errors, e.g. from move test

Only on-chain. We render things separately in the unit test infrastructure (and based off the VM state in the unit tests) for move test. So this is really only a stop-gap for on-chain errors until we have a GraphQL-backed CLI.

Some(format!("{command}{suffix} command aborted within {error}"))
}

fn parse_abort_status_string(
Copy link
Member

Choose a reason for hiding this comment

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

sad. panda.

Copy link
Member

Choose a reason for hiding this comment

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

Doc comments would also be good here, to express the requisite remorse for having to resort to:

  • Trying to extract any kind of structure out of this unstructured error response,
  • regexp parsing a context-free grammar.

I felt your pain reading this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea... 😢 not the most beautiful of functions lol

Will add a doc comment saying to kill this the moment we can/have a GraphQL-backed CLI :)

---
Clever-error-utf8
---
Error executing transaction '7YmFWxA3WKzKF7Bp3sJisD83EZTGXFosD4TKmq8ughLY': 1st command aborted within function '0xb691b7e8d182a5d5c8dd834b234a2bd55d9c42210cba8e852cd134586a74390e::clever_errors::clever_aborter' at line 19. Aborted with 'ENotFound' -- 'Element not found in vector 💥 🚀 🌠'
Copy link
Member

Choose a reason for hiding this comment

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

Beautiful. I can't wait to see the first error on mainnet that uses emoji.

@@ -444,6 +445,14 @@ impl<'a> Disassembler<'a> {
}
}

fn preview_string(s: &str) -> String {
if s.len() <= 4 {
Copy link
Member

Choose a reason for hiding this comment

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

This is a pet peeve of mine 😅 , but this bound should be 6 to make sure the preview is always smaller than the input string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed! Very good spot. Will update :)

@tzakian tzakian force-pushed the tzakian/cli-clever-errors branch from e4d4115 to 0c55e82 Compare June 18, 2024 23:43
@tzakian tzakian enabled auto-merge (squash) June 18, 2024 23:44
@tzakian tzakian merged commit aa1ad98 into main Jun 19, 2024
47 checks passed
@tzakian tzakian deleted the tzakian/cli-clever-errors branch June 19, 2024 00:14
tx-tomcat pushed a commit to tx-tomcat/sui-network that referenced this pull request Jul 29, 2024
## Description 

Enables clever error rendering in the Sui CLI.

It also abstracts out some of the rendering logic for constants into
their own file so it can be reused across the graphql and Sui CLI
renderer.

This also updates the Move disassembler to use this new (nicer!)
renderer to that we support rendered values for constants in the Move
disassembler instead of just strings in constants.

## Test plan 

Added new tests for this to the Sui CLI, and updated/added additional
tests for the disassembler.

---

## 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: 
- [ ] GraphQL: 
- [X] CLI: Add better rendering for Move aborts, and added support for
rendering clever errors.
- [ ] Rust SDK:
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.

4 participants