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

Bring back UI for execution #401

Merged
merged 29 commits into from
Sep 5, 2023
Merged

Bring back UI for execution #401

merged 29 commits into from
Sep 5, 2023

Conversation

zoeyTM
Copy link
Contributor

@zoeyTM zoeyTM commented Aug 24, 2023

fixes #259

@zoeyTM zoeyTM force-pushed the feat/execution-ui branch from 0c38d68 to cff68f4 Compare August 24, 2023 04:11
Base automatically changed from refactor/update-state-and-errors to development August 24, 2023 14:56
@zoeyTM zoeyTM force-pushed the feat/execution-ui branch from cff68f4 to 48a2637 Compare August 24, 2023 18:51
@zoeyTM
Copy link
Contributor Author

zoeyTM commented Aug 28, 2023

Added a VerboseEventHandler

Screen Shot 2023-08-28 at 2 13 29 PM

@alcuadrado
Copy link
Member

alcuadrado commented Aug 28, 2023

Great progress! Good idea to start with the verbose logging listener.

Now that we decided to have independent ExecutionEvents that don't necessarily map 1:1 with JournalMessages, I think we can apply this simplification: Instead of having the deployment loader emit the ExecutionEvents, emit them directly whenever needed.

I think this would mean emitting them in:

  • Deployer._getOrInitializeDeploymentState when a new deployment gets started.
  • Deployer.deploy, to emit the BATCH_INITIALIZE event.
  • Maybe in the execution engine to emit an event when each batch started (maybe I missed this event)
  • All the places where applyNewMessage is called, except maybe in the Wiper.

Also, how annoying is it to have to use this weird syntax?

public [ExecutionEventType.RUN_START](event: RunStartEvent): void {
// ....
}

@zoeyTM
Copy link
Contributor Author

zoeyTM commented Aug 29, 2023

@alcuadrado ah okay, i can update that!

As for the weird syntax, we don't technically have to do it that way. We could also do

public RUN_START(event: RunStartEvent): void {
// ....
}

I just did it the current way because I figured it was one less thing we have to update if we ever change one of the enum values but maybe I'm overthinking it. Now that I'm typing this out, though, I'm thinking there's not really a reason besides convention that we couldn't do something like:

export enum ExecutionEventType {
  RUN_START = "runStart",
  WIPE_EXECUTION_STATE = "wipeExecutionStart",
  // etc...
}

which would make the handler functions just normal function names. Thoughts?

Switched out the usage of flat to the core function.

Added in doc comments to meet api-extractor rules.
kanej and others added 8 commits August 31, 2023 17:11
On a rerun no batches are executed. Show the deployed addresses anyway.
This involved adding a deployment complete event, that passes the
deployment result.

This will be extended to failure cases in future commits.
Instead of showing all batches at once. Only display a batch if at least
one future has started (excluding the first batch).

We may want to enhance this by tracking the current batch in the ui
based on the batch events.
@kanej kanej marked this pull request as ready for review September 5, 2023 13:04
@kanej kanej merged commit 4569b88 into development Sep 5, 2023
0 of 6 checks passed
@kanej kanej deleted the feat/execution-ui branch September 5, 2023 13:06
@kanej kanej mentioned this pull request Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Show UI during execution
3 participants