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] Improve the way we display PTB info in table output #15957

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

stefan-mysten
Copy link
Contributor

@stefan-mysten stefan-mysten commented Jan 28, 2024

Description

Improves the way we display PTB info in table output. Previously, the inputs would output all the data in a long String, which breaks the table.
Before
image

After
image

Test Plan

Visual inspection.


If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section.

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

Improved the display of transactions of type Transaction Kind: Programmable in the CLI's output.

Copy link

vercel bot commented Jan 28, 2024

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 Jan 28, 2024 5:54pm
sui-core ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 28, 2024 5:54pm
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 28, 2024 5:54pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Visit Preview Jan 28, 2024 5:54pm
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Jan 28, 2024 5:54pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Jan 28, 2024 5:54pm

Comment on lines +871 to +877
let mut xs = items.into_iter();
let Some(x) = xs.next() else {
return Ok(());
};
write!(f, "{x}")?;
for x in xs {
write!(f, "{sep}{x}")?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improvement as per @amnn's suggestion in a previous PR.

Comment on lines +408 to +413
writeln!(writer, "Transaction Kind: Epoch Change")?;
writeln!(writer, "New epoch ID: {}", e.epoch)?;
writeln!(writer, "Storage gas reward: {}", e.storage_charge)?;
writeln!(writer, "Computation gas reward: {}", e.computation_charge)?;
writeln!(writer, "Storage rebate: {}", e.storage_rebate)?;
writeln!(writer, "Timestamp: {}", e.epoch_start_timestamp_ms)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cosmetic changes, removed a space between name and :.

writeln!(writer, "Transaction Kind : Programmable")?;
write!(writer, "{p}")?;
write!(writer, "Transaction Kind: Programmable")?;
write!(writer, "{}", crate::displays::Pretty(p))?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main change where the new Pretty display is being used for Programmable TXs.

@lavindir
Copy link
Contributor

Does any of this change the replay command output or is this a completely copied over version?

In the picture above, input objects is missing a space between the second and third row to align the third row for all input types!

Copy link
Contributor

@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.

Adding to Laura's question -- is there a way for us to re-use the implementation that Replay uses for this? (I'm thinking it may not be easy because these are JSONRPC types instead of core types, and then there's a question of whether we even want to invest in cleaning those up if they are going to be replaced soon, but worth checking).

@stefan-mysten
Copy link
Contributor Author

Does any of this change the replay command output or is this a completely copied over version?

I have not tried to see if replay gets broken, but I am certain it does not. And the reason for that is that this only changes the output for SuiProgrammableTransactionBlock that is part of SuiTransactionBlockResponse type. Only those commands that return a SuiTransactionBlockResponse will be affected, so replay will not be affected.

Adding to Laura's question -- is there a way for us to re-use the implementation that Replay uses for this? (I'm thinking it may not be easy because these are JSONRPC types instead of core types, and then there's a question of whether we even want to invest in cleaning those up if they are going to be replaced soon, but worth checking).

That's exactly the issue. It is unfortunate that I couldn't use the already implementation of Pretty from sui-types. From what I see, we kinda seem to be able to go from a sui-types struct to a sui-json-rpc-types, but not the other way around.

Long term, I think we need to review how these things should work and how GraphQL would affect the sui-json-rpc-types. Probably, as you @amnn mentioned some time ago, we need to do all this pretty printing stuff in the cli crates; so we should have this Pretty stuff for most types that the CLI uses.

Finally, I need this for Wed demo as the broken table is really nasty when executing transactions. Happy to address the feedback as we want after Wed,

Copy link
Contributor

@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.

Let me accept to unblock, but note @laura-makdah's comment on alignment of input types.

@stefan-mysten stefan-mysten merged commit 3474ab4 into MystenLabs:main Jan 30, 2024
41 checks passed
@stefan-mysten stefan-mysten deleted the ptb_display branch January 30, 2024 00:03
@lavindir
Copy link
Contributor

Style wise, I would add a space in the input object table between the object type and the proceeding value, for all types, such that the third column is aligned.

I would also remove the bracketing from the gas payment, and reserve bracketing for data that has both a repeated and grouped pagination

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

3 participants