Foundry UI #3392

franciszekjob · 2025-05-22T13:21:09Z

Towards #3022

Stack:

Introduced changes

Introduce foundry-ui crate
- Add UI, Message

Checklist

This reverts commit b3be726.

This reverts commit 4808311.

This reverts commit dd2fafb.

This reverts commit 108f17c.

crates/foundry-ui/src/lib.rs

cptartur

Looks good I think, left some comments about possible improvements

cptartur · 2025-05-23T16:23:35Z

crates/foundry-ui/src/lib.rs

+    #[must_use]
+    pub fn from_flag(json: bool) -> Self {
+        if json {
+            OutputFormat::Json
+        } else {
+            OutputFormat::Human
+        }
+    }


I'd just allow creating it by passing an enum and force the caller to deal with that. from_flag seems bit weird.

Note that this enum was previously in print.rs, I've just moved it. from_flag corresponds to --json flag so I think it actually makes sense.

I'd still prefer we remove that

Any specific reason? I don't really see a point to remove it - it's the simplest way to create it based on bool:

starknet-foundry/crates/sncast/src/main.rs

Line 217 in 32302f6

let output_format = OutputFormat::from_flag(cli.json);

I think it's unnecessary indirection for something this simple. The usage you posted is obvious but from other places, it's not obvious at all what from_flag method does since arguments name aren't easily visible to the caller.

Why not

let output_format = if cli.json { OutputFormat::Json } else { OutputFormat::Human };

at the caller site? We aren't saving any code from doing this.

Why not at the caller site?

Imo it's not a good practice to write ifs logic directly at the level of main function, even if they are simple.

Let's follow your reasoning and assume we introduce other simple flags in future.

fn main() -> Result<()> { let cli = Cli::parse(); let numbers_format = NumbersFormat::from_flags(cli.hex_format, cli.int_format); let output_format = if cli.json { OutputFormat::Json } else { OutputFormat::Human }; let other_value = cli.other_flag { ... } else { ... } ... }

Then our main function would get cluttered over the time.

I'm open for discussion but the arguments you've provided don't really convince me 😅

@ksew1 @kkawula wdyt about this?

I prefer the solution that @franciszekjob proposed, it takes away logic from main function, let's stick to it

crates/foundry-ui/src/lib.rs

crates/foundry-ui/src/message.rs

crates/sncast/src/response/account/create.rs

cptartur

Responded to comments

… into 3022-foundry-ui

crates/sncast/src/response/explorer_link.rs

kkawula · 2025-05-27T09:33:36Z

crates/sncast/src/response/print.rs

@@ -154,6 +127,14 @@ impl From<Value> for OutputData {
    }
 }

+impl<T: CommandResponse + Serialize> From<&T> for OutputData {


CommandResponse + Serialize = CommandResponse
Is it correct?

Yes, it's used in text and json in SncastMessage

kkawula · 2025-05-27T09:34:36Z

crates/sncast/src/starknet_commands/account/deploy.rs

@@ -63,6 +64,7 @@ pub async fn deploy(
            keystore_path_,
        )
        .await
+        .map(Into::into)


can we get rid of this map somehow?

Note that now we return a separate type AccountDeployResponse which is constructed from InvokeResponse, hence the usage of into.

crates/sncast/src/starknet_commands/multicall/run.rs

crates/foundry-ui/src/message.rs

crates/foundry-ui/src/lib.rs

ksew1 · 2025-05-27T12:15:24Z

crates/foundry-ui/src/lib.rs

+
+    /// Get the output format of this [`UI`] instance.
+    #[must_use]
+    pub fn output_format(&self) -> OutputFormat {


I don't like how we are using this. We are doing ui.output_format() -> format message according to output_format -> print message. The flow should be i give message to ui and it knows how to format it.

I think the idea is you create a UI instance per output format (json or human readable) and you pass it through the application. To me it makes sense that the UI has a (immutable) state like this.

@ksew1 you're right, output format should be held only in UI instance and not exposed - note this will be removed in #3396, which focuses on refactoring print-related code in cast 😄

cptartur · 2025-05-27T16:12:43Z

crates/foundry-ui/src/lib.rs

+    #[must_use]
+    pub fn from_flag(json: bool) -> Self {
+        if json {
+            OutputFormat::Json
+        } else {
+            OutputFormat::Human
+        }
+    }


I think it's unnecessary indirection for something this simple. The usage you posted is obvious but from other places, it's not obvious at all what from_flag method does since arguments name aren't easily visible to the caller.

Why not

let output_format = if cli.json { OutputFormat::Json } else { OutputFormat::Human };

at the caller site? We aren't saving any code from doing this.

cptartur · 2025-05-27T16:13:52Z

crates/foundry-ui/src/lib.rs

+
+    /// Get the output format of this [`UI`] instance.
+    #[must_use]
+    pub fn output_format(&self) -> OutputFormat {


I think the idea is you create a UI instance per output format (json or human readable) and you pass it through the application. To me it makes sense that the UI has a (immutable) state like this.

cptartur · 2025-05-27T16:16:51Z

crates/foundry-ui/src/message.rs

+impl Message for &str {
+    fn text(&self) -> String {
+        (*self).to_string()
+    }
+}
+
+impl Message for String {
+    fn text(&self) -> String {
+        self.to_string()
+    }
+}


How does that play with the default conversion to json? The user will just get the encoded string?

Not sure how we can enforce this on the type level (or if), but for json output maybe it makes sense to return objects always?

What I mean is that maybe there is a reason for the output to always be of the format

SomeObject { "key1": "value1" }

and with these default impls the user will get just "string text".

cc @kkawula @ksew1

What I mean is that maybe there is a reason for the output to always be of the format

You mean that json should be pretty-printed? If so, I don't think we really need to introduce this. Keep in mind that we've been always returning outputs in json format not pretty printed, so I don't see the reason to change it.

cptartur · 2025-05-27T16:24:11Z

crates/sncast/src/response/explorer_link.rs

    };

    if let Ok(provider) = explorer.unwrap_or_default().as_provider(network) {
-        response.print_links(provider);
+        return Some(response.print_links(provider));


Maybe the method should be renamed. It doesn't seem like it is printing anything anymore

cptartur · 2025-05-27T16:31:53Z

crates/foundry-ui/src/lib.rs

+#[derive(Debug)]
+pub struct UI {
+    output_format: OutputFormat,
+    // TODO(3395): Add state here, that can be used for spinner
+}
+
+impl UI {
+    /// Create a new [`UI`] instance configured with the given output format.
+    #[must_use]


Now that I think of it, we should probably define a trait for UI and implement it here.

So you can type our function as accepting the UI trait, not the specific implementation of it.
The benefit will be you will be able to mock the UI in tests or provide an implementation that allows us to just capture what is printed so we can assert it (e.g. for tests that show warnings about snforge_std).

cc @kkawula @ksew1

franciszekjob added 29 commits May 20, 2025 18:14

wip: Add foundry-ui crate; Integration with sncast

0e607b9

wip: Add Message impl for more responses

9335d7b

wip: Reorganize files

5b1f2ba

wip: Adjust response structs to fix state file tests

59ac3e7

Remove unused code

5ea5b34

wip: Fix call tests

44d1562

wip: Fix account deploy tests

d557ada

fix: cast scripts

ef4350c

fix: more more tests

bf6795c

wip: Adjust displayed text for command

c2f506b

fix: show-config tests

b3be726

Remove command field from command response structs

5d08566

Uncomment block explorer tests

88946cc

Fix multicall tests

5de4910

Fix more tests

2f1e758

Revert "fix: show-config tests"

259f085

This reverts commit b3be726.

Update OutputData.to_lines

dd2fafb

Update OutputData.to_lines

4808311

Revert "Update OutputData.to_lines"

8d36d8b

This reverts commit 4808311.

Revert "Update OutputData.to_lines"

0fe74ff

This reverts commit dd2fafb.

Fix show config tests

ecec1c1

Restore command field in command response structs

108f17c

Revert "Restore command field in command response structs"

018d456

This reverts commit 108f17c.

Further improvements of Ui

10eae2e

Code cleanup

f8ab2e7

Refactor structs; Reorganize files

9e352bd

Code cleanup

b08b311

Code cleanup

ce3231f

Add generic impl Message for CastMessage<T>

ddd776b

franciszekjob commented May 22, 2025

View reviewed changes

crates/foundry-ui/src/lib.rs Show resolved Hide resolved

franciszekjob added 2 commits May 23, 2025 01:29

Add todos

b6a80a6

Code cleanup

96a4cd0

This was referenced May 22, 2025

Add message components #3393

Open

Use foundry UI in sncast #3396

Open

franciszekjob marked this pull request as ready for review May 22, 2025 23:58

franciszekjob requested a review from a team as a code owner May 22, 2025 23:58

franciszekjob requested review from cptartur, kkawula and ksew1 May 22, 2025 23:58

cptartur reviewed May 23, 2025

View reviewed changes

Rename Ui to UI

e557e3d

franciszekjob requested a review from cptartur May 25, 2025 16:55

franciszekjob mentioned this pull request May 26, 2025

Use foundry UI in forge #3402

Open

5 tasks

Fix typo in sncast message

2d832ec

cptartur reviewed May 26, 2025

View reviewed changes

franciszekjob added 3 commits May 26, 2025 15:55

Remove default text implementation in Message

1619e75

Merge branch 'master' of https://github.com/foundry-rs/starknet-foundry…

9625877

… into 3022-foundry-ui

Rename max_fee to estimated_fee

7c9f94d

This was referenced May 26, 2025

Remove print utils from shared #3405

Open

Use foundry uI in docs crate #3409

Open

franciszekjob requested a review from cptartur May 26, 2025 23:27

kkawula reviewed May 27, 2025

View reviewed changes

Apply code review suggestions

31f4325

franciszekjob requested a review from kkawula May 27, 2025 09:59

ksew1 reviewed May 27, 2025

View reviewed changes

Apply part of code review suggestions

391e0b5

cptartur reviewed May 27, 2025

View reviewed changes

Add todo

690b81d

franciszekjob requested a review from ksew1 May 27, 2025 17:12

Foundry UI #3392

Are you sure you want to change the base?

Foundry UI #3392

Conversation

franciszekjob commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Introduced changes

Checklist

Uh oh!

Uh oh!

cptartur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

franciszekjob May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

franciszekjob May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

franciszekjob May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cptartur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

franciszekjob May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

franciszekjob commented May 22, 2025 •

edited

Loading

franciszekjob May 25, 2025 •

edited

Loading

franciszekjob May 26, 2025 •

edited

Loading

franciszekjob May 27, 2025 •

edited

Loading

franciszekjob May 27, 2025 •

edited

Loading