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

add Action to high-level trigger updateState #7621

Merged
merged 6 commits into from
Oct 9, 2020

Conversation

S11001001
Copy link
Contributor

@S11001001 S11001001 commented Oct 8, 2020

We could trivially add an identity monad to the updateState signature and then build from there, but we might as well use it for something right away, so ActionState moves to the standard library (so get, put can be unified) and the new monad gets one of those to control the high-level trigger state.

We also introduce DA.Action.State.Class.

CHANGELOG_BEGIN
- [Triggers] The ``updateState`` function now returns a ``TriggerStateA``.  This
  is an action like ``TriggerA``, but doesn't permit emitting commands.  Instead
  of taking the state as an argument and returning a new state, you can
  manipulate the state with ``get``, ``put``, and ``modify``.  Any existing
  ``updateState`` can be ported by replacing ``s -> expr`` in the lambda
  expression with ``-> modify $ \s ->``, and then made to look nicer from there
  as desired.
  See `issue #7621 <https://github.com/digital-asset/daml/pull/7621>`__.
CHANGELOG_END

Fixes #7620.

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

@S11001001 S11001001 self-assigned this Oct 8, 2020
@cocreature
Copy link
Contributor

I’m not sure this is a good idea. Currently we have a very clear separation:

  • initialize and updateState are pure functions, they both work together and neither of them can do any effects
  • rule is the effectful thing where all effects should end up in

At least for now, most users don’t worry about initialize and updateState.

With this PR, we are changing things a bit:

  • There is now a (necessary) inconsistency between initialize and updateState. It’s far less obvious that the two go together now just from looking at the types.
  • It’s not immediately obvious that you can only modify the state in updateState potentially leading to users trying to submit commands from the update.

I realize that for intermediate DAML programmers this is probably not going to be an issue but I’m a bit worried that we’re going to cause confusion for people looking at triggers for the first time with fairly little benefit.

Are you planning to include more effects than just the state update?

@S11001001
Copy link
Contributor Author

@cocreature See #7620 for the overall reasoning for introducing this now.

There is now a (necessary) inconsistency between initialize and updateState. It’s far less obvious that the two go together now just from looking at the types.

That's good, because the impression from several of the items in #7392 is that it is really updateState and rule that ought to go together, and initialize that ought to be the odd man out.

Consider

The commands in flight are only exposed in the rule. Exposing them in updateState might be useful.

It seems that it would be most clear and consistent if the way you accessed them would always be via the getCommandsInFlight polymorphic action.

Or consider the example that this PR very nearly touches:

The user-defined state is exposed inconsistently. Currently, you get access to it everywhere but you cannot modify it from the rule.

Supposition 1: you wish to retrieve and modify the state from both updateState and rule. Supposition 2: it is often unused in simple rules, making it desirable to get it out of the way, with only a mention of () in the type for those cases. My conclusion: it would be desirable to work with the state in a consistent way between updateState and rule, via get and put.

Are you planning to include more effects than just the state update?

I considered only having this PR introduce the identity monad, honestly, but put in State as a proof of concept. As described, I believe this is the path to the most natural solution to several issues with the API we intend to tackle, and the set of effects would grow accordingly, just not in this PR.

@S11001001
Copy link
Contributor Author

It’s not immediately obvious that you can only modify the state in updateState potentially leading to users trying to submit commands from the update.

As we wish to provide multiple similar capabilities between the updateState and rule, it seems that either we can make a different way to do each thing in each function, or we can risk giving the above impression (to be corrected by damlc). I would rather pay the latter price.

@S11001001 S11001001 added this to the Trigger Service milestone Oct 9, 2020
@cocreature
Copy link
Contributor

Alright, having thought about this for a few hours, I think I’m convinced. Maybe we can make the docs a bit clearer to avoid any confusion from users around TriggerStateA not being able to submit commands.

CHANGELOG_BEGIN
- [Triggers] The ``updateState`` function now returns a ``TriggerStateA``.  This
  is an action like ``TriggerA``, but doesn't permit emitting commands.  Instead
  of taking the state as an argument and returning a new state, you can
  manipulate the state with ``get``, ``put``, and ``modify``.  Any existing
  ``updateState`` can be ported by replacing ``s -> expr`` in the lambda
  expression with ``-> modify $ \s ->``, and then made to look nicer from there
  as desired.
  See `issue #7621 <https://github.com/digital-asset/daml/pull/7621>`__.
CHANGELOG_END
Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

@S11001001 S11001001 merged commit 664a0c0 into master Oct 9, 2020
@S11001001 S11001001 deleted the 7620-trigger-updatestate-monad branch October 9, 2020 17:56
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.

high-level Trigger updateState monad
2 participants