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

Test authorization within Exercise choice. #11246

Merged
merged 11 commits into from
Oct 18, 2021
Merged

Test authorization within Exercise choice. #11246

merged 11 commits into from
Oct 18, 2021

Conversation

nickchapman-da
Copy link
Contributor

@nickchapman-da nickchapman-da commented Oct 14, 2021 •
edited
Loading

Test that authorization of an exercise choice, collects authority from both

  • the contract signatory, and
  • the choice controller.

@nickchapman-da nickchapman-da added the component/daml-engine DAML-LF Engine & Interpreter label Oct 14, 2021
@nickchapman-da nickchapman-da force-pushed the nick-test-choice-authorization branch from 0d1f6b4 to cdce1a9 Compare October 14, 2021 14:36
@nickchapman-da nickchapman-da marked this pull request as ready for review October 14, 2021 16:35
cocreature
cocreature reviewed Oct 15, 2021
View reviewed changes
daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthPropagationSpec.scala
interpretResult
}

"Create(T1)" - {
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we testing authorization of a create in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(1) Party to gain confidence in this testing setup.
(2) But I wonder if all tests from AuthorizationSpec would be better done at this level: Currently these just check that calls to CheckAuthorization return as expected. But doing the testing here would also check the calls to the CheckAuthorization functions are made in the correct places!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds very sensible to do everything in here but maybe let’s leave the reshuffling for another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthPropagationSpec.scala Outdated
Comment on lines 230 to 231
inside(interpretResult) { case Right(_) =>
}
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
inside(interpretResult) { case Right(_) =>
}
inside(interpretResult) shouldBe a[Right[_, _]]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks

I made various other cleanups which clarify the tests I hope.

daml-lf/engine/src/test/scala/com/digitalasset/daml/lf/engine/AuthPropagationSpec.scala
// Test authorization of an exercise choice collects authority from both
// the contract signatory and the choice controller

"Exercise(Choice1 of T1 to create T2)" - {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we also want an exercise in an exercises. Specifically the interesting case is that in the body of the inner exercise you only have the authorization of the signatories and controllers meaning you can drop authorization compared to the outer exercise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Added this.

@nickchapman-da nickchapman-da marked this pull request as draft October 15, 2021 15:47
@nickchapman-da nickchapman-da marked this pull request as ready for review October 18, 2021 10:38
cocreature
cocreature approved these changes Oct 18, 2021
View reviewed changes
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.

Thanks!

@nickchapman-da nickchapman-da merged commit 2db8c13 into main Oct 18, 2021
@nickchapman-da nickchapman-da deleted the nick-test-choice-authorization branch October 18, 2021 11:33
remyhaemmerle-da
remyhaemmerle-da reviewed Oct 18, 2021
View reviewed changes
Copy link
Collaborator

@remyhaemmerle-da remyhaemmerle-da left a comment

Choose a reason for hiding this comment

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

Nice.

azure-pipelines bot pushed a commit that referenced this pull request Oct 20, 2021
release 1.18.0-snapshot.20211019.8113.0.8ff347d8
c3ee08e 
This PR has been created by a script, which is not very smart
and does not have all the context. Please do double-check that
the version prefix is correct before merging.

@sofiafaro-da is in charge of this release.

Commit log:
```
8ff347d Expand type synonyms lazily (#11282)
2fd200f KV: support contextual logging in KeyValueConsumption [KVL-1143] (#11288)
f4df1cc converting server errors to proper client errors (#11184)
525e4ce Add GrpcStatus.toProto(JavaStatus) -> ScalaStatus conversion [KVL-1143] (#11292)
9b00a1a Rotate release rotation (#11291)
dd09c38 Upgrade rules-nodejs (#11290)
87f1418 [Short] Remove unnecessary traits from ApiSubmissionServiceSpec (#11254)
6b65a72 Merge npm install steps in create-daml-app tests (#11287)
a9f6afb kvutils: Rename `VersionedOffset` to `KVOffset`. (#11286)
98cf8d8 KV: introduce v2 error codes behind a CLI switch [KVL-1140] (#11224)
46f6877 Increase time model skew limits (#11273)
8f94cff kvutils: Use `VersionedOffsetBuilder` where possible, and introduce `VersionedOffsetMutator`. [KVL-1154] (#11277)
81fde97 Bazel: Call `_wrap_rule` directly when building the Scala REPL rule. (#11279)
edb2b04 Document scaladoc usage in Bazel (#11278)
6255837 Data dependencies support for reexports (#11147)
2fc7490 [Self-service error codes] Adapt error factories [DPP-656] (#11270)
b1a6b11 ledger-api-test-tool: Add static time awareness [KVL-1156] (#11266)
243c120 Add an LF typechecking benchmark (#11276)
2db8c13 Test authorization within Exercise choice. (#11246)
ec58ed6 Typecheck nested LF type applications more efficiently (#11253)
a940016 Add debugRaw corresponding to traceM in Haskell (#11259)
a988579 Fix Scala repl targets (#11272)
e4808f6 Extract UtilLF module from daml-lf-conversion to its own library (#11263)
38712c0 sandbox-next - Disable participant command deduplication [KVL-1156] (#11264)
82fa229 Add daml-util-ghc lib (#11260)
68a93cf kvutils - Use the ledger configured time for command dedup [KVL-1149] (#11239)
c8cede0 Bump startup time in create-daml-app tests (#11261)
4fac87f Bump the schema version for the JSON API (#11252)
5507670 update NOTICES file (#11256)
0063b10 Retry faster during startup (#11255)
04feb40 Improve reset service tests (#11240)
b3375fd update compat versions for 1.18.0-snapshot.20211013.8071.0.514e8b50 (#11237)
9ed1eb3 Address security notice on `ansi-regex` (#11243)
387d0e8 Make compatibility migrations `eternal` (#11242)
be899b3 Extract rejection_reason.proto from daml_kvutils.proto [KVL-1090] (#11235)
a1d94e1 LF: Create special case class for VersionedContractInstance (#11127)
b738988 Release 1.18 snapshot (#11234)
3c26852 LF: Use template Id in exercise node of fixed choice. (#11229)
139b6f3 CI: Set `PROJ_DIR` inside the bash lib, not outside. (#11236)
ce64cb2 interfaces: Do some TODOs (#11231)
7a88c7d trigger-service: dev-mode-unsafe flag (#11233)
909a1bf [DPP-417][DDP-612] Adapt ApiSubmissionService to support V2 error codes (#11052)
```
Changelog:
```
- [JSON API] Several kinds of gRPC server errors are now reported with
  associated HTTP statuses; for example, a Daml-LF interpreter error now
  returns a 400 instead of a 500, and an exercise on an archived contract
  returns a 409 Conflict instead of a 500.  Errors internal to JSON API
  (e.g. internal assertion failures) are no longer detailed in the HTTP
  response; their details are only logged.
  See `issue #11184 <https://github.com/digital-asset/daml/pull/11184>`__.

- [Daml Stdlib] Add `debugRaw` as a convenience wrapper around
  `traceRaw` when used inside a do-block. `debugRaw` compares to
  `debug` like `traceRaw` compares to `trace` meaning it expects a
  `Text` instead of calling `show` on an expression.

sandbox-next - Disable participant command deduplication

kvutils - Command deduplication uses the configured time model (static/wall) and not running always using wall-clock
[JSON API] Solving a bug that could cause the JSON API to return
correct result if a contract with the same key is observed twice
required a schema change. The JSON API data needs to be dropped
and the query store needs to reset. If you are migrating from a
previous version, either reset your database manually or start
the HTTP JSON API with one of the options that regenerate the
schema (`create-only`, `create-if-needed-and-start`, `create-and-start`).
```

CHANGELOG_BEGIN
CHANGELOG_END
sofiafaro-da pushed a commit that referenced this pull request Oct 20, 2021
@azure-pipelines
release 1.18.0-snapshot.20211019.8113.0.8ff347d8 (#11300)
f315a90 
This PR has been created by a script, which is not very smart
and does not have all the context. Please do double-check that
the version prefix is correct before merging.

@sofiafaro-da is in charge of this release.

Commit log:
```
8ff347d Expand type synonyms lazily (#11282)
2fd200f KV: support contextual logging in KeyValueConsumption [KVL-1143] (#11288)
f4df1cc converting server errors to proper client errors (#11184)
525e4ce Add GrpcStatus.toProto(JavaStatus) -> ScalaStatus conversion [KVL-1143] (#11292)
9b00a1a Rotate release rotation (#11291)
dd09c38 Upgrade rules-nodejs (#11290)
87f1418 [Short] Remove unnecessary traits from ApiSubmissionServiceSpec (#11254)
6b65a72 Merge npm install steps in create-daml-app tests (#11287)
a9f6afb kvutils: Rename `VersionedOffset` to `KVOffset`. (#11286)
98cf8d8 KV: introduce v2 error codes behind a CLI switch [KVL-1140] (#11224)
46f6877 Increase time model skew limits (#11273)
8f94cff kvutils: Use `VersionedOffsetBuilder` where possible, and introduce `VersionedOffsetMutator`. [KVL-1154] (#11277)
81fde97 Bazel: Call `_wrap_rule` directly when building the Scala REPL rule. (#11279)
edb2b04 Document scaladoc usage in Bazel (#11278)
6255837 Data dependencies support for reexports (#11147)
2fc7490 [Self-service error codes] Adapt error factories [DPP-656] (#11270)
b1a6b11 ledger-api-test-tool: Add static time awareness [KVL-1156] (#11266)
243c120 Add an LF typechecking benchmark (#11276)
2db8c13 Test authorization within Exercise choice. (#11246)
ec58ed6 Typecheck nested LF type applications more efficiently (#11253)
a940016 Add debugRaw corresponding to traceM in Haskell (#11259)
a988579 Fix Scala repl targets (#11272)
e4808f6 Extract UtilLF module from daml-lf-conversion to its own library (#11263)
38712c0 sandbox-next - Disable participant command deduplication [KVL-1156] (#11264)
82fa229 Add daml-util-ghc lib (#11260)
68a93cf kvutils - Use the ledger configured time for command dedup [KVL-1149] (#11239)
c8cede0 Bump startup time in create-daml-app tests (#11261)
4fac87f Bump the schema version for the JSON API (#11252)
5507670 update NOTICES file (#11256)
0063b10 Retry faster during startup (#11255)
04feb40 Improve reset service tests (#11240)
b3375fd update compat versions for 1.18.0-snapshot.20211013.8071.0.514e8b50 (#11237)
9ed1eb3 Address security notice on `ansi-regex` (#11243)
387d0e8 Make compatibility migrations `eternal` (#11242)
be899b3 Extract rejection_reason.proto from daml_kvutils.proto [KVL-1090] (#11235)
a1d94e1 LF: Create special case class for VersionedContractInstance (#11127)
b738988 Release 1.18 snapshot (#11234)
3c26852 LF: Use template Id in exercise node of fixed choice. (#11229)
139b6f3 CI: Set `PROJ_DIR` inside the bash lib, not outside. (#11236)
ce64cb2 interfaces: Do some TODOs (#11231)
7a88c7d trigger-service: dev-mode-unsafe flag (#11233)
909a1bf [DPP-417][DDP-612] Adapt ApiSubmissionService to support V2 error codes (#11052)
```
Changelog:
```
- [JSON API] Several kinds of gRPC server errors are now reported with
  associated HTTP statuses; for example, a Daml-LF interpreter error now
  returns a 400 instead of a 500, and an exercise on an archived contract
  returns a 409 Conflict instead of a 500.  Errors internal to JSON API
  (e.g. internal assertion failures) are no longer detailed in the HTTP
  response; their details are only logged.
  See `issue #11184 <https://github.com/digital-asset/daml/pull/11184>`__.

- [Daml Stdlib] Add `debugRaw` as a convenience wrapper around
  `traceRaw` when used inside a do-block. `debugRaw` compares to
  `debug` like `traceRaw` compares to `trace` meaning it expects a
  `Text` instead of calling `show` on an expression.

sandbox-next - Disable participant command deduplication

kvutils - Command deduplication uses the configured time model (static/wall) and not running always using wall-clock
[JSON API] Solving a bug that could cause the JSON API to return
correct result if a contract with the same key is observed twice
required a schema change. The JSON API data needs to be dropped
and the query store needs to reset. If you are migrating from a
previous version, either reset your database manually or start
the HTTP JSON API with one of the options that regenerate the
schema (`create-only`, `create-if-needed-and-start`, `create-and-start`).
```

CHANGELOG_BEGIN
CHANGELOG_END

Co-authored-by: Azure Pipelines Daml Build <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@remyhaemmerle-da remyhaemmerle-da remyhaemmerle-da left review comments

@cocreature cocreature cocreature approved these changes

Assignees
No one assigned
Labels
component/daml-engine DAML-LF Engine & Interpreter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nickchapman-da @cocreature @remyhaemmerle-da