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.

Sign up for GitHub

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

Jump to bottom

daml-lf: freeze version 1.7 #3340

Merged
merged 2 commits into from
Nov 7, 2019
Merged

daml-lf: freeze version 1.7 #3340

merged 2 commits into from
Nov 7, 2019

Conversation

remyhaemmerle-da
Copy link
Collaborator

@remyhaemmerle-da remyhaemmerle-da commented Nov 5, 2019 •
edited
Loading

fixes: #2289

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
  • Add a line to the release notes, if appropriate
  • 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.

@remyhaemmerle-da remyhaemmerle-da force-pushed the daml-lf-1.7 branch 3 times, most recently from c2c8b0e to afb3aeb Compare November 5, 2019 15:16
@remyhaemmerle-da remyhaemmerle-da marked this pull request as ready for review November 5, 2019 15:28
remyhaemmerle-da
remyhaemmerle-da commented Nov 5, 2019
View reviewed changes
language-support/java/codegen/BUILD.bazel Outdated Show resolved Hide resolved
language-support/java/codegen/BUILD.bazel Outdated Show resolved Hide resolved
ledger/test-common/BUILD.bazel Outdated Show resolved Hide resolved
hurryabit
hurryabit reviewed Nov 5, 2019
View reviewed changes
Copy link
Contributor

@hurryabit hurryabit left a comment

Choose a reason for hiding this comment

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

The Haskell bits look good. (I don't agree with a recent change in 1.dev, but that's not part of this PR.)

daml-lf/spec/daml-lf-1.rst
Version: 1.dev
..............

* **Change** Transaction submitter must be in the contract key
Copy link
Contributor

Choose a reason for hiding this comment

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

When did we decide to do this? I'm not convinced requiring the submitter of a fetchByKey to be a maintainer of the key is a good idea. This definitely hurts the compositionality of workflows. I also don't see the technical necessity of this change. Being a stakeholder of the contract should absolutely be enough. If you know a contract with the right key exists, then it must be the unique contract with this key. If you don't see a contract with such a key, you can't submit the transaction anyway. (I'm not talking about lookupByKey here!)

cc @bame-da @gerolf-da

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, this restriction is pretty much against the spirit of #2311, which we all seem to have agreed on.

Copy link
Collaborator Author

@remyhaemmerle-da remyhaemmerle-da Nov 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is not recent. This was done by francesco. See #1866 already referenced in #2311.

Copy link
Collaborator Author

@remyhaemmerle-da remyhaemmerle-da Nov 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I just moved the comment from line 227/232 to line 246/252

@remyhaemmerle-da remyhaemmerle-da force-pushed the daml-lf-1.7 branch 3 times, most recently from 28ce72f to 6005f0c Compare November 6, 2019 08:43
SamirTalwar
SamirTalwar reviewed Nov 6, 2019
View reviewed changes
daml-lf/language/daml-lf.bzl Outdated
lf_dev_version = "1.dev"

lf_versions = [lf_stable_version, lf_stable_version, lf_dev_version]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
lf_versions = [lf_stable_version, lf_stable_version, lf_dev_version]
lf_versions = [lf_stable_version, lf_latest_version, lf_dev_version]

@remyhaemmerle-da remyhaemmerle-da added this to the Maintenance milestone Nov 7, 2019
@mergify mergify bot merged commit d39d797 into master Nov 7, 2019
@mergify mergify bot deleted the daml-lf-1.7 branch November 7, 2019 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SamirTalwar SamirTalwar SamirTalwar left review comments

@gerolf-da gerolf-da gerolf-da approved these changes

@leo-da leo-da Awaiting requested review from leo-da

@S11001001 S11001001 Awaiting requested review from S11001001

@rautenrieth-da rautenrieth-da Awaiting requested review from rautenrieth-da

@stefanobaghino-da stefanobaghino-da Awaiting requested review from stefanobaghino-da

@hurryabit hurryabit Awaiting requested review from hurryabit

Assignees
No one assigned
Labels
component/daml-lf DAML-LF
Projects
None yet
Milestone
Maintenance
Development

Successfully merging this pull request may close these issues.

Add Numeric type to LF
4 participants
@remyhaemmerle-da @SamirTalwar @hurryabit @gerolf-da