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

fix: MASM debug logs #17

Draft
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

TomasArrachea
Copy link
Collaborator

No description provided.

@@ -549,7 +549,8 @@ impl<R: FeltRng> Client<R> {
where
T: IntoIterator<Item = (Word, Vec<Felt>)>,
{
TransactionScript::compile(program, inputs, TransactionKernel::assembler())
let assembler = TransactionKernel::assembler().with_debug_mode(self.in_debug_mode);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, could we instantiate by checking if TransactionExecutor is using debug mode instead of keeping a variable in the Client?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At the moment there is no public method on the TransactionExecutor to check the debug mode, so this would require changes on miden-base. Should we make those changes?

@TomasArrachea TomasArrachea force-pushed the tomasarrachea-masm-logs-from-client branch 2 times, most recently from 2a5f6bc to 39d6a75 Compare January 9, 2025 20:15
@@ -28,7 +28,9 @@ required-features = ["testing", "concurrent"]

[dev-dependencies]
assert_cmd = { version = "2.0" }
predicates = { version = "3.0" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this used only for the contains()? If so, I think we could remove this and do the check manually, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The assert_cmd stdout method expects a predicates_core::Predicate object, so I don't think there is an easy way of testing it manually without using the predicates crate

@@ -0,0 +1 @@
pub mod common;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a comment here saying that we export utility functions to use in different crates

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done!

Copy link
Collaborator

@tomyrd tomyrd left a comment

Choose a reason for hiding this comment

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

There's another instance of TransactionKernel::assembler() inside TransactionScriptBuilder::build_script_with_sections we should also change it so that it uses the client's debug flag (we could add it to TransactionScriptBuilder::new).

tests/Cargo.toml Outdated
Comment on lines 22 to 23
uuid = { version = "1.10", features = ["serde", "v4"] }
toml = { version = "0.8" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should switch these so that they are in alphabetical order

@igamigo
Copy link
Collaborator

igamigo commented Jan 10, 2025

There's another instance of TransactionKernel::assembler() inside TransactionScriptBuilder::build_script_with_sections we should also change it so that it uses the client's debug flag (we could add it to TransactionScriptBuilder::new).

We should probably have a single private Client function that creates the assembler for all scenarios so that we don't miss anything in the future

@TomasArrachea TomasArrachea force-pushed the tomasarrachea-masm-logs-from-client branch from 549786b to c869a1c Compare January 10, 2025 14:52
@TomasArrachea TomasArrachea reopened this Jan 10, 2025
@TomasArrachea TomasArrachea force-pushed the tomasarrachea-masm-logs-from-client branch from c869a1c to 41887a8 Compare January 10, 2025 18:12
@TomasArrachea TomasArrachea force-pushed the tomasarrachea-masm-logs-from-client branch from 41887a8 to 49792f5 Compare January 10, 2025 20:32
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.

3 participants