feat(CLI): Typed rendering for call output - #2179
Conversation
|
Hi @igamigo! Quick question about the changelog for this PR: there is already an entry saying “Added miden-cli call command for invoking account procedures directly from the CLI”. Should we reuse that existing entry for this PR as well, or to add a new changelog entry that explicitly mentions typed rendering for the call output? |
I think we can reuse it and add this PR link to the same entry since it's very related, but not a strong opinion so if you prefer otherwise that's fine too. |
|
Can you expand the PR description explaining the design a bit and how it relates to the issue? And/or maybe add some examples of how this renders. I know there is some context there already but the actual implementation and the scope has not been fully discussed AFAICT |
|
Hi @igamigo thanks for your comment! I updated the PR description. There's one thing I just realized, and because of it I'm turning this into a draft: I accidentally tested it against a compiler branch that isn't merged into next yet, this branch is the one that stores the high-level types inside the debug sections. My bad, I didn't check that my |
There was a problem hiding this comment.
Approve direction, but let's hold non-draft on the items below. Thanks @marijamijailovic!
Phase 1 from #2098 is covered.
Before flipping to non-draft though:
- Test coverage. See inline on
package_types/mod.rs—bool,u32,u64are priority types from #2098 but have no unit test. - Compiler dependency. See inline on
tests/cli.rs— the typed path only triggers because the test stubs the debug sections in by hand. - CHANGELOG. The check is still red and I can't drop an inline since
CHANGELOG.mdisn't in the diff. Per your thread with @igamigo, extending the existing #1943 entry sounds right, something like:
Added
miden-cli callcommand for invoking account procedures directly from the CLI (#1943), with typed argument encoding and typed result rendering when the package carries debug type info (#2179).
Follow-ups to track separately, not here:
- Phase 2
SchemaTyperendering. - Module naming:
package_typesreads ambiguous next to the manifest's ownFunctionType. Happy to be overruled.
| /// `call` command exercises its typed encode/decode path. Only this proc is annotated; `add` | ||
| /// and `set_value` resolve to `None` in `TypedProcInfo` and stay on the raw path. | ||
| fn build_take_account_id_debug_sections() -> ( | ||
| miden_mast_package::debug_info::DebugFunctionsSection, |
There was a problem hiding this comment.
This helper is load-bearing for the typed test, and it exists because the official compiler does not emit DebugTypeInfo::Function entries yet (per your draft-state note above). That means the only end-to-end coverage of the typed path runs against hand-built debug sections.
Could you link the corresponding compiler PR or open a tracking issue so we have an explicit gate for "pioneers running cargo miden build against the published toolchain will see typed output"? I want to be sure we don't ship this and then quietly have everyone fall back to raw rendering until the compiler catches up.
a74dc47 to
927cc8f
Compare
|
I’ve pushed a follow-up that addresses the review comments on the typed
|
|
cc @BrianSeong99 I am tagging you here so you are aware of this one |
|
Hi @igamigo! Would it make sense to start the review while keeping this PR in draft until the compiler changes land, and then make this one “ready” once the compiler is ready as well? |
igamigo
left a comment
There was a problem hiding this comment.
Overall I'm not entirely sure the new package_debug_info should be part of miden_client. It seems like it encompasses a bunch of package/compiler-related helpers and is not really used in the miden-client crate, only the CLI. In this sense, I wonder if it should be placed either in the CLI directly or as a separate compiler crate, as it's very much independent of the codebase here.
| pub use self::encode::parse_felt_token; | ||
| use self::encode::{arg_token_count, encode_tokens}; | ||
| pub use self::errors::PackageDebugInfoError; |
There was a problem hiding this comment.
nit: Let's move the pub use to be below the mod definitions (ie, below line 4 and 5). Let's also move all the imports to be above the mod definitions
| /// Encodes `tokens` as a flat felt vector matching the procedure's parameter types. `word` | ||
| /// and `account-id` each parse from a single hex token; other structs expect one token per | ||
| /// leaf field. | ||
| pub fn encode_args(&self, tokens: &[String]) -> Result<Vec<Felt>, PackageDebugInfoError> { |
There was a problem hiding this comment.
Do we need this function here? It seems like this is an application responsibility. For example, perhaps a CLI would be interested in taking strings and parsing them, but I don't think we need to make it a concern of TypedProcInfo. Perhaps it can take a list of felts and validate them against the proc info
Thanks, that’s a good point. I agree this probably live in I'm thinking about moving the whole typed encoder/decoder into This way the split would be: the compiler writes the debug sections, Let me know what do you think? And we can tag in people from the miden-vm / compiler side who can share their opinion. |
|
Hey @marijamijailovic / @Keinberger, just a heads up, if we want this on the |
|
This PR is blocked for now.
After the fix is introduce, I will:
So this PR stays in draft until then. |
5c1cb85 to
69c7a49
Compare
|
Hey @igamigo, rebased on main, miden-vm PR is open — 0xMiden/miden-vm#3276 |
The above issue is resolved. |
|
With the issue resolved are we now ready to move this PR out of draft and have it reviewed? Or were we waiting on the VM patch release? |
Thanks for bumping this up! Now that the |
|
Hi @marijamijailovic, would we be ready to move this PR out of draft or is there anything missing? |
Hi, not yet, as per comment, I'm now waiting for @bitwalker PR to land first, and then to update how |
Drop the local package_debug_info module and consume the typed encoder/decoder from miden-mast-package's debug_info::typed instead.
|
@marijamijailovic 0.25.8 has landed, and the new debugger has shipped, so I think all the necessary pieces are in place for you now |
69c7a49 to
707e7cd
Compare
igamigo
left a comment
There was a problem hiding this comment.
LGTM! Holding off on approving until we have upstream dependencies merged but leaving some mostly minor comments for now.
Also, can you update the PR description? It seems to have gotten stale.
| #[error("client has not been synced yet")] | ||
| #[diagnostic( | ||
| code(cli::not_synced), | ||
| help("Run `{} sync` first.", client_binary_name().display()) | ||
| )] | ||
| NotSynced, |
| miden-client-sqlite-store = { workspace = true } | ||
| miden-debug = { optional = true, workspace = true } | ||
| miden-mast-package = { workspace = true } | ||
| miden-protocol = { workspace = true } |
There was a problem hiding this comment.
We should not need this import. AFAIK all types you are using are already being re-exported from miden-client
|
|
||
| if found_without_signature { | ||
| return Err(missing_signature(procedure_name)); | ||
| } |
There was a problem hiding this comment.
Previously it seems that such procedures would be callable with u64 args and a stack dump. Is this a regression? If so we might want to add the "breaking" tag to the changelog.
| /// Part of the transaction kernel's assertion message for a transaction that changes nothing. | ||
| /// | ||
| /// The assertion carries a message rather than a code the executor exposes, so this is matched as | ||
| /// text. It only decides whether an explanatory line is printed: if the kernel ever rewords it, | ||
| /// the line stops appearing and nothing else changes. | ||
| const EMPTY_TRANSACTION_ASSERTION: &str = "neither changed the account state"; |
There was a problem hiding this comment.
I think we should be able to remove this and do something like
let is_empty_transaction = matches!(
&e,
ClientError::TransactionExecutorError(
TransactionExecutorError::TransactionProgramExecutionFailed(exec_err),
) if ERR_EPILOGUE_EXECUTED_TRANSACTION_IS_EMPTY.matches_execution_error(exec_err)
);Which is still not great but at least it does not depend on substring matching against a local const.
This PR extends the call command: it now looks for type info in the package's debug sections, and if it's there it prints a typed signature and decodes the result (for now account-id, bool, word, structs) instead of showing raw felts. If there's no type info it just prints the raw stack like before, so nothing changes for existing packages.
It reads two sections from the package - DEBUG_FUNCTIONS (the procedure name, its params and signature) and DEBUG_TYPES (the type definitions). With those, the args you pass on the CLI get encoded according to the parameter types (e.g. an account-id hex token expands to two felts, a word to four), and the returned felts get decoded back into the same types for printing.
Example, you'll get something like:
Closes #2098