SCP-1751: Keep simulation state after compilation error #4081

sjoerdvisscher · 2021-10-08T10:53:45Z

Pre-submit checklist:

Branch
- Tests are provided (if possible)
- Commit sequence broadly makes sense
- Key commits have useful messages
- Relevant tickets are mentioned in commit messages
- Formatting, materialized Nix files, PNG optimization, etc. are updated
PR
- Self-reviewed the diff
- Useful pull request description
- Reviewer requested

jhbertra

Looks like a reasonable approach, but keeping these two values in sync seems a bit fragile... I would suggest creating a helper to updated them both, and replacing all instances of assign _compilationResult x with it:

assignCompilationRestul :: forall m. MonadState State m => WebCompilationResult -> m Unit
assignCompilationRestul newResult =
  assign _compilationResult newResult
  oldSuccesfulResult <- use _lastSuccessfulCompilationResult
  case newResult, oldSuccesfulResult of
    Success (Right _), _ -> assign _lastSuccessfulCompilationResult newResult
    _, Success (Right _) -> pure unit
    _, _ -> assign _lastSuccessfulCompilationResult newResult

I believe that should take care of all possible cases, and now you only need to call one function to update both in a consistent manner!

jhbertra · 2021-10-08T16:09:49Z

plutus-playground-client/src/MainFrame/State.purs

@@ -332,6 +334,7 @@ handleAction CompileProgram = do
    Nothing -> pure unit
    Just contents -> do
      oldCompilationResult <- use _compilationResult
+      oldSuccessfulCompilationResult <- use _lastSuccessfulCompilationResult
      assign _compilationResult Loading


Here, I think we should also set oldSuccessfulCompilationResult to Loading if it is currently NotAsked or Failure

Suggested change

assign _compilationResult Loading

assign _compilationResult Loading

case _lastSuccessfulCompilationResult of

NotAsked -> assign _lastSuccessfulCompilationResult Loading

Failure _ -> assign _lastSuccessfulCompilationResult Loading

_ -> pure unit

jhbertra · 2021-10-08T16:14:29Z

plutus-playground-client/src/MainFrame/State.purs

@@ -342,6 +345,7 @@ handleAction CompileProgram = do
          when (isSuccess newCompilationResult) do


can we add a pattern to set the _lastSuccessfulCompilationResult to Failure if it is currently Loading?

Failure error -> case oldSuccessfulCompilationResult of Loading -> assign _lastSuccessfulCompilationResult $ Failure error _ -> pure unit

sjoerdvisscher · 2021-10-12T13:14:40Z

Thanks! That reminded me of a blogpost I read recently (can't find it rn) that you can do this in the setter of a lens.

I don't get your other suggestions. I think they would break the fix again. The point is that the check starting from line 367 only goes through if there are 2 successful compilations and the second one is different from the first.

jhbertra · 2021-10-13T12:16:53Z

plutus-playground-client/src/MainFrame/Lenses.purs

-_compilationResult = _Newtype <<< prop (SProxy :: SProxy "compilationResult")
+_compilationResult = _Newtype <<< lens g s
+  where
+  g r = r.compilationResult


🚼Learning about plutus @jhbertra .
Are you waiting for SProxy compliationResult class to update and therefore you must rely on a local var declaration?

jhbertra

LGTM. I just have a question about the logic still

jhbertra · 2021-10-13T12:24:05Z

plutus-playground-client/src/MainFrame/Lenses.purs

+  s r c =
+    if is (_Success <<< _Right) c then
+      r { compilationResult = c, lastSuccessfulCompilationResult = c }
+    else
+      r { compilationResult = c }


This will still only update the field if the result is a success - is this what you want? If so, maybe it would be better to make lastSuccessfulCompilationResult field a Maybe instead of a WebData. Otherwise, you might want to consider this instead:

Suggested change

s r c =

if is (_Success <<< _Right) c then

r { compilationResult = c, lastSuccessfulCompilationResult = c }

else

r { compilationResult = c }

s r c =

case r. ^. _lastSuccessfulCompilationResult, c of

_, Sucess (Right _) -> -- assign if it is a successful result

r { compilationResult = c, lastSuccessfulCompilationResult = c }

Sucess (Right _), _ -> -- else do not assign if the last result was a success

r { compilationResult = c }

_, _ -> -- else assign regardless (for initial loading, failure states to display)

r { compilationResult = c, lastSuccessfulCompilationResult = c }

This is actually what I want. But the Maybe suggestion is a good one, that cleaned up (and clarified) the code a bit.

jhbertra · 2021-10-13T12:25:10Z

plutus-playground-client/src/MainFrame/Lenses.purs


 _successfulCompilationResult :: Traversal' State CompilationResult
 _successfulCompilationResult = _compilationResult <<< _Success <<< _Right <<< _InterpreterResult <<< _result

+_lastSuccessfulCompilationResult :: Lens' State (WebData (Either InterpreterError (InterpreterResult CompilationResult)))


To discourage setting this field directly, I'd suggest making it a Getter instead

Suggested change

_lastSuccessfulCompilationResult :: Lens' State (WebData (Either InterpreterError (InterpreterResult CompilationResult)))

_lastSuccessfulCompilationResult :: Getter' State (WebData (Either InterpreterError (InterpreterResult CompilationResult)))

I actually tried this, but this caused a runtime error! Not sure why...

Yikes... that shouldn't happen. I've seen runtime errors happen when switching branches and my webpack cache got stale. I can't think of another reason why that would happen though.

…nResult is updated

michaelpj · 2021-10-21T13:40:32Z

plutus-apps has split off from plutus. Apologies for the inconvenience, but this needs to be ported to the new repository here: https://github.com/input-output-hk/plutus-apps

sjoerdvisscher changed the title ~~Keep simulation state after compilation error~~ SCP-1751: Keep simulation state after compilation error Oct 8, 2021

sjoerdvisscher requested a review from jhbertra October 8, 2021 10:54

Keep simulation state after compilation error

1b5e1e8

sjoerdvisscher force-pushed the SCP-1751-keep-simulation-state branch from 18eac7f to 1b5e1e8 Compare October 8, 2021 10:55

jhbertra approved these changes Oct 8, 2021

View reviewed changes

Merge branch 'master' into SCP-1751-keep-simulation-state

c1cdefe

sjoerdvisscher force-pushed the SCP-1751-keep-simulation-state branch from 9fa8d71 to 968ca2f Compare October 12, 2021 13:06

sjoerdvisscher requested a review from jhbertra October 12, 2021 13:18

jhbertra reviewed Oct 13, 2021

View reviewed changes

jhbertra approved these changes Oct 13, 2021

View reviewed changes

sjoerdvisscher force-pushed the SCP-1751-keep-simulation-state branch from 968ca2f to bd089e0 Compare October 14, 2021 11:09

Automatically assign to lastSuccessfulCompilationResult if compilatio…

d17083a

…nResult is updated

sjoerdvisscher force-pushed the SCP-1751-keep-simulation-state branch from bd089e0 to d17083a Compare October 14, 2021 11:25

sjoerdvisscher added 3 commits October 14, 2021 13:25

Merge branch 'master' into SCP-1751-keep-simulation-state

a9ce8e3

Simplify

db6dbd8

Merge branch 'master' into SCP-1751-keep-simulation-state

71ccca5

michaelpj closed this Oct 21, 2021

sjoerdvisscher mentioned this pull request Oct 21, 2021

SCP-1751: Keep simulation state after compilation error IntersectMBO/plutus-apps#36

Merged

8 tasks

kwxm deleted the SCP-1751-keep-simulation-state branch February 2, 2022 08:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCP-1751: Keep simulation state after compilation error #4081

SCP-1751: Keep simulation state after compilation error #4081

sjoerdvisscher commented Oct 8, 2021 •

edited

Loading

jhbertra left a comment •

edited

Loading

jhbertra Oct 8, 2021

jhbertra Oct 8, 2021

sjoerdvisscher commented Oct 12, 2021

jhbertra Oct 13, 2021

kcw-grunt Oct 15, 2021

jhbertra left a comment

jhbertra Oct 13, 2021

sjoerdvisscher Oct 14, 2021

jhbertra Oct 13, 2021

sjoerdvisscher Oct 13, 2021

jhbertra Oct 14, 2021

michaelpj commented Oct 21, 2021

-      assign _compilationResult Loading
+      assign _compilationResult Loading
+      case _lastSuccessfulCompilationResult of
+        NotAsked -> assign _lastSuccessfulCompilationResult Loading
+        Failure _ -> assign _lastSuccessfulCompilationResult Loading
+        _ -> pure unit

		@@ -342,6 +345,7 @@ handleAction CompileProgram = do
		when (isSuccess newCompilationResult) do

	_lastSuccessfulCompilationResult :: Lens' State (WebData (Either InterpreterError (InterpreterResult CompilationResult)))
	_lastSuccessfulCompilationResult :: Getter' State (WebData (Either InterpreterError (InterpreterResult CompilationResult)))

SCP-1751: Keep simulation state after compilation error #4081

SCP-1751: Keep simulation state after compilation error #4081

Conversation

sjoerdvisscher commented Oct 8, 2021 • edited Loading

jhbertra left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjoerdvisscher commented Oct 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhbertra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj commented Oct 21, 2021

sjoerdvisscher commented Oct 8, 2021 •

edited

Loading

jhbertra left a comment •

edited

Loading