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

Debugger. #28441

Draft
wants to merge 28 commits into
base: mainnet
Choose a base branch
from
Draft

Debugger. #28441

wants to merge 28 commits into from

Conversation

mikebenfield
Copy link
Collaborator

No description provided.

@@ -110,6 +115,7 @@ impl LeoError {
UtilError(error) => error.error_code(),
LastErrorCode(_) => unreachable!(),
Anyhow(_) => "SnarkVM Error".to_string(), // todo: implement error codes for snarkvm errors.
InterpreterHalt(_) => "Interpreter Halt".to_string(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we give these a proper error code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure. My thought is that unlike the other error variants, InterpreterHalt doesn't necessarily represent undesired/unexpected behavior - the user may be stepping through code perfectly well expecting that an assert may trigger (or whatever).

leo/cli/commands/interpret.rs Outdated Show resolved Hide resolved
interpreter/src/lib.rs Outdated Show resolved Hide resolved
interpreter/src/lib.rs Outdated Show resolved Hide resolved
interpreter/src/lib.rs Outdated Show resolved Hide resolved
interpreter/src/lib.rs Outdated Show resolved Hide resolved
integers::Integer as SvmIntegerParam,
};

type SvmAddress = SvmAddressParam<TestnetV0>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want these types parameterized over the Network?

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 the interpreter only works with TestnetV0. It's not clear to me whether it's important that it works with the other Networks. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We just rolled some updates where TestnetV0 and MainnetV0 can differ in behavior depending on the block height. I could see that as being a potential case, but not much else.

interpreter/src/cursor.rs Outdated Show resolved Hide resolved
interpreter/src/cursor.rs Outdated Show resolved Hide resolved
}

#[derive(Clone, Debug, Default, Eq, PartialEq, Hash)]
pub struct Future(pub Vec<AsyncExecution>);
Copy link
Collaborator

Choose a reason for hiding this comment

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

That way I've been thinking about Futures is that they are a single AsyncExecution, whose arguments can be other AsyncExecutions. I'm sure I'll understand as I make it further down, but is there a reason why it's a vec here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What I have is that a Future is a sequence of AsyncExecutions, and when an async function calls await, it collects that Future's AsyncExecutions into its Future, which it will return when it finishes executing.

It's not clear to me that there is any advantage in having a tree of executions as opposed to just a sequence. Is there?

};
}

if let Some(value) = match expression {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Organizational nit. Consider splitting the functional components (expression evaluation) and control flow of the REPL loop for easier maintenance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, I thought I did that. Where do you consider them to be interwoven?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah that's my bad on the lack of clarity. I was thinking splitting them across different files.

@d0cd d0cd changed the title Interpreter. Debugger. Nov 20, 2024
@d0cd
Copy link
Collaborator

d0cd commented Nov 20, 2024

We should be also allow the debugger to start in a non-Leo context.

@d0cd
Copy link
Collaborator

d0cd commented Nov 20, 2024

It would be nice for the cursor for the REPL loop to show where we are currently at

> let foo: u8 = 1u8;

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.

2 participants