Adding wireframe contract setup workflow. #2773

merivale · 2021-02-24T12:03:11Z

Work in progress, but this seems as good a moment as any to open a PR... A few comments:

It's not possible yet, in the contract setup, to create a contact and immediately assign it to a role - that can wait for another PR. Otherwise, all the contract setup workflow is in place.
Validation of number inputs is a pain. I had the same problem with the Plutus playground and never quite got to the bottom of it. But I think we've probably got enough validation here for the prototype.
It looks as though there is a proper Template.State submodule, but there isn't. I've just separated out some template-related MainFrame.State code into a separate module for readability. Compare with Contract.State. Not sure which way is better. 🤷‍♂️

@palas, I've changed Marlowe.Extended slightly: added a getParties function and changed the default timeout value from zero to one.

UPDATE: While I was responding to Pablo's comments, I thought I might as well include the changes to template metadata as well. Sorry that makes this PR a bit larger.

Also I forgot to mention: Although we are not including contract drafts in the prototype, this PR does give you one draft for free. I.e. while you are setting up a contract you can go away and do something else - the roles and template parameters will still be around if you go back to setting up a contract from that same template later. (In concrete terms: the template state is only overwritten if you start setting up a different template.)

merivale · 2021-02-24T12:05:16Z

marlowe-dashboard-client/src/MainFrame/Lenses.purs

 _templates = prop (SProxy :: SProxy "templates")

 _subState :: Lens' State (Either PickupState WalletState)
 _subState = prop (SProxy :: SProxy "subState")

-- This isn't a Traversal' in any meaningful sense (that I can see), but a Traversal'


Turns out it is an affine traversal. 🤓 So I'm removing this comment.

palas

Looks good, I wrote one type design issue and then it is just lints

palas · 2021-02-24T13:08:10Z

marlowe-dashboard-client/src/Contract/State.purs

-initialState :: Slot -> Contract -> State
-initialState slot contract =
+defaultState :: State
+defaultState = mkInitialState zero Close


Using Close and zero as default for when there is no contract is a smell for me (is a bit like using null in Java). I think this should be wrapped in some kind of Maybe type or custom data type with a NotStarted constructor or something like that. It is a bit more work, but I think that will make things much easier in the long run, especially when debugging later.

Yeah, we have a general problem here. David and I have been experimenting with different options, and so far have found having a default state (that we know will never be used) is the least awkward. Otherwise you have to handle the Nothing cases in the view functions, even though the view functions will never be called in that case. But I agree it is "wrong". Going to leave it for now if you don't mind, and we can revisit it as we go along...

it's nice that this is a separate function from mkInitialState as well as it indicates the situation somewhat.

palas · 2021-02-24T13:10:41Z

marlowe-dashboard-client/src/MainFrame/State.purs

@@ -69,6 +71,8 @@ mkWalletState pubKeyHash =
  , menuOpen: false
  , screen: ContractsScreen Running
  , card: Nothing
+  , templateState: Template.defaultState
+  , contractState: Contract.defaultState


Could wrap both of these in a subtype in a Maybe or something like that, because we only have templateState and contractState either both at the same time or none

palas · 2021-02-24T13:15:24Z

marlowe-dashboard-client/src/MainFrame/State.purs

+  case mCurrentTemplate of
+    Just currentTemplate
+      | currentTemplate == template -> pure unit
+    _ -> assign (_walletState <<< _templateState) $ Template.mkInitialState template


I think 187-190 can be written as:

when (mCurrentTemplate /= Just template) $ assign (_walletState <<< _templateState) $ Template.mkInitialState template

So it can, thank you.

palas · 2021-02-24T13:19:31Z

marlowe-dashboard-client/src/MainFrame/State.purs

+    let
+      templateContent = view _templateContent templateState
+    let
+      mContract = toCore $ fillTemplate templateContent extendedContract


I think 198-205 can be written as:

let extendedContract = view (_template <<< _extendedContract) templateState templateContent = view _templateContent templateState mContract = toCore $ fillTemplate templateContent extendedContract in

Without do or pure unit

Oops, don't know how that pure unit got in there. Must have been during a rewrite when I wasn't paying full attention. Thanks. 👍

palas · 2021-02-24T13:23:35Z

web-common/src/Halogen/Extra.purs

+
+  mainToSub mainState = case preview traversal mainState of
+    Just submoduleState -> submoduleState
+    _ -> submoduleDefaultState


I think 91-93 could also be written:
mainToSub mainState = fromMaybe submoduleDefaultState $ preview traversal mainState
or even:
mainToSub = fromMaybe submoduleDefaultState <<< preview traversal
Not sure about parenthesis in last one

So it can. Nice.

palas · 2021-02-24T13:28:58Z

web-common-marlowe/src/Marlowe/Extended.purs

+  getParties (Scale _ val) = getParties val
+  getParties (ChoiceValue choId) = getParties choId
+  getParties (Cond obs lhs rhs) = getParties obs <> getParties lhs <> getParties rhs
+  getParties _ = Set.empty


I prefer to specify all the clauses (not using catch-all underscores) because that way if we modify Marlowe types we will get an error here. As it is now, we will get a default behaviour of no parties which is harder to notice. But this is just my opinion, we can leave it as is, no big deal

Okay, I don't have strong feelings on this so I'll go with your preference.

palas · 2021-02-24T13:30:45Z

web-common-marlowe/src/Marlowe/Extended.purs

+  getParties :: a -> Set S.Party
+
+instance arrayHasParties :: HasParties a => HasParties (Array a) where
+  getParties as = foldMap (\a -> getParties a) as


(\a -> getParties a) is equivalent to getParties

palas

Sorry one more thing...

palas · 2021-02-24T13:44:31Z

marlowe-dashboard-client/src/Template/State.purs

+mkRoleWallets contract = fromFoldable $ Set.map (\name -> Tuple name "") (getRoleNames contract)
+
+getRoleNames :: Contract -> Set String
+getRoleNames contract = Set.delete "" $ Set.map roleName (getParties contract)


This delete here removes also Role "" which I don't think is right. We can just use mapMaybe and have roleName return a Maybe String. Using "", again, is a smell to me, like using null

Yes, that was bugging me too. Good idea with mapMaybe. 👍

shmish111

just a few suggestions but can be in next PR

shmish111 · 2021-02-24T15:20:35Z

marlowe-dashboard-client/src/Contract/State.purs

-initialState :: Slot -> Contract -> State
-initialState slot contract =
+defaultState :: State
+defaultState = mkInitialState zero Close


it's nice that this is a separate function from mkInitialState as well as it indicates the situation somewhat.

shmish111 · 2021-02-24T15:27:50Z

marlowe-dashboard-client/src/MainFrame/State.purs

+handleAction (ContractAction contractAction) = handleSubAction (_walletState <<< _contractState) ContractAction Contract.defaultState (Contract.handleAction contractAction)
+
+-- there must be a nicer way to get the current state than by manually
+-- piecing together each of its properties, but I haven't found it :(


given that we are using row types, can't you just use the current state?

state <- get

Well that is exactly the function I wanted. And did not know existed. 😄

shmish111 · 2021-02-24T15:43:41Z

marlowe-dashboard-client/src/MainFrame/View.purs

-            Just (ContractCard contract) -> [ ContractAction <$> contractDetailsCard contract ]
+            Just NewContractForRoleCard -> []
+            Just ContractSetupConfirmationCard -> [ contractSetupConfirmationCard ]
+            Just ContractCard -> [ ContractAction <$> contractDetailsCard contractState ]
            Nothing -> []


I think this is

(flip foldMap card) \cardType -> case cardType of CreateWalletCard -> [ newContactCard newWalletNicknameKey wallets ] ...

Not sure if it's nicer or not, I just didn't like the Just x so many times and I spotted that we have Nothing -> [] i.e. Nothing -> mempty i.e. Foldable f => Monoid m => f a -> m which is foldMap :: forall m a. Monoid m => (a -> m) -> f a -> m where the (a -> m) is the pattern match without the Justs everywhere

shmish111 · 2021-02-24T15:53:19Z

marlowe-dashboard-client/src/Template/State.purs

+mkRoleWallets contract = fromFoldable $ Set.map (\name -> Tuple name "") (getRoleNames contract)
+
+getRoleNames :: Contract -> Set String
+getRoleNames contract = Set.mapMaybe roleName (getParties contract)


why not combine these 2 functions, it doesn't seem like they are doing that much

fromFoldable $ Set.mapMaybe getRoleEntry (getParties contract) where getRoleEntry (PK _) = Nothing getRoleEntry (Role tokenName) = Just (Tuple tokenName "")

yeah, I though getRoleNames might be useful somewhere else, but probably not

shmish111 · 2021-02-24T15:57:06Z

marlowe-dashboard-client/src/Template/View.purs

+    templateContent = view _templateContent state
+  in
+    div
+      []


I think we've generally agreed on using div_ where possible these days (though I'm not that bothered)

Yes, but I'm expecting most of these divs will need classes when we start styling, so I've not been that fussed about this for now.

shmish111 · 2021-02-24T15:57:46Z

marlowe-dashboard-client/src/Template/View.purs

+contractSetupScreenHeader setupScreen contractNickname =
+  div_
+    [ div
+        []


shmish111 · 2021-02-24T15:58:35Z

marlowe-dashboard-client/src/Template/View.purs

+        [ onClick $ const $ Just $ ToggleCard TemplateLibraryCard ]
+        [ text "< Library quick access" ]
+    , button
+        [ onClick $ const $ Just $ SetScreen $ ContractSetupScreen ContractParametersScreen


we should really define onClick <<< const <<< Just somewhere (although I don't know a good name)

shmish111 · 2021-02-24T15:59:50Z

marlowe-dashboard-client/src/Template/View.purs

+        [ text "< Library quick access" ]
+    , button
+        [ onClick $ const $ Just $ SetScreen $ ContractSetupScreen ContractParametersScreen
+        , disabled $ not $ roleWalletsAreValid roleWallets wallets


https://github.com/purescript-halogen/purescript-halogen/blob/v5.0.0/src/Halogen/HTML/Properties.purs#L228

didn't know that existed - thanks

shmish111 · 2021-02-24T16:02:16Z

web-common-marlowe/src/Marlowe/Extended.purs

+  getParties :: a -> Set S.Party
+
+instance arrayHasParties :: HasParties a => HasParties (Array a) where
+  getParties = foldMap (\a -> getParties a)


foldMap getParties

Adding wireframe contract setup workflow.

fd5f7e1

merivale requested a review from shmish111 February 24, 2021 12:03

merivale commented Feb 24, 2021

View reviewed changes

merivale requested a review from palas February 24, 2021 12:08

Running fix-purty.

58a3ce6

palas requested changes Feb 24, 2021

View reviewed changes

Responding to review comments and adding enhanced contract metadata.

b456685

palas approved these changes Feb 24, 2021

View reviewed changes

shmish111 approved these changes Feb 24, 2021

View reviewed changes

merivale merged commit 68ff8a8 into master Feb 25, 2021

kwxm deleted the marlowe-dash-contract-setup branch June 16, 2021 06:27

Adding wireframe contract setup workflow. #2773

Adding wireframe contract setup workflow. #2773

Conversation

merivale commented Feb 24, 2021 • edited

Choose a reason for hiding this comment

palas 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

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

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

palas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmish111 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merivale commented Feb 24, 2021 •

edited