LF: V1 Contract ID check in Preprocessor #10687
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
remyhaemmerle-da
force-pushed
the
remy-lf-v0-cid
branch
2 times, most recently
from
e9f7122 to
e046a3f
Compare
This PR makes possible to reject V0 contract IDs during preprocessing. CHANGELOG_BEGIN CHANGELOG_END
remyhaemmerle-da
force-pushed
the
remy-lf-v0-cid
branch
from
e046a3f to
463c1e5
Compare
remyhaemmerle-da
requested review from
aherrmann-da,
cocreature,
stefanobaghino-da and
a team
as code owners
remyhaemmerle-da
force-pushed
the
remy-lf-v0-cid
branch
from
463c1e5 to
8379e3d
Compare
cocreature
approved these changes
Aug 30, 2021
| // TODO: https://github.com/digital-asset/daml/issues/10504 | ||
| // switch the default to true | ||
| requireSuffixedGlobalCids: Boolean = false, | ||
| requireV1ContractId: Boolean = false, |
There was a problem hiding this comment.
maybe forbidV0 would be more future proof if we ever add v2?
There was a problem hiding this comment.
Ok so in canton, I would rename use use of requireV1ContractId to forbidV0ContractId.
And no need to use requireV1ContractIdSuffix, right, @remyhaemmerle-da ?
There was a problem hiding this comment.
@oliverse-da you should use both: forbidV0ContractId to make sure you never get a global v0 cid. requireV1ContractIdSuffix to make sure that all global v1 cids have a suffix.
| override def message: String = | ||
| s"Provided value exceeds maximum nesting level of ${Value.MAXIMUM_NESTING}" | ||
| } | ||
|
|
||
| final case class NonSuffixedCid(cid: Value.ContractId.V1) extends Error { | ||
| final case class IllegalContractId(cid: Value.ContractId) extends Error { |
There was a problem hiding this comment.
should we include the reason here and tell users that it failed because of lack of suffix or because they used v0?
There was a problem hiding this comment.
I did not feel it was necessary, in the sens that if it's happen it means the user try to feed to the ledger ID that it has cooked himself. If the user want to shoot himself in a foot, I do not see why I shoot tell him which foot is it.
If you really think it is useful, I will add the reason.
There was a problem hiding this comment.
fair enough, I tend to lean on the side of including too much information rather than too little in case we need it later but I don’t feel all that strongly about this one.
cocreature
reviewed
Aug 30, 2021
| new preprocessing.CommandPreprocessor(compiledPackages.interface, requiredCidSuffix = false) | ||
| new preprocessing.CommandPreprocessor( | ||
| compiledPackages.interface, | ||
| requireV1ContractId = false, |
There was a problem hiding this comment.
Can we not enforce v1 cids in Daml Studio?
There was a problem hiding this comment.
will try.
cocreature
reviewed
Aug 30, 2021
| // switch the default to true | ||
| requireSuffixedGlobalCids: Boolean = false, | ||
| requireV1ContractId: Boolean = false, | ||
| requireSuffixedGlobalContractId: Boolean = false, |
There was a problem hiding this comment.
did you drop the todo intentionally?
There was a problem hiding this comment.
Yes. After our discussion last week where you told me you are not concern about misusage of the flags (because we have only 3 actively maintain ledgers) I just thought the default behavior does not really matter.
remyhaemmerle-da
force-pushed
the
remy-lf-v0-cid
branch
from
8379e3d to
463c1e5
Compare
6 tasks
remyhaemmerle-da
force-pushed
the
remy-lf-v0-cid
branch
from
e6c08c6 to
78d8f0d
Compare
azure-pipelines bot
pushed a commit
that referenced
this pull request
Sep 1, 2021
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. @aherrmann-da is in charge of this release. Commit log: ``` f058c2f DPP-368 clean up flags (#10711) 90ad24f [Divulgence pruning] Prune immediate divulgence [DPP-513] (#10691) 183934b Command dedup: add columns to completions in append-only schemas [KVL-1057] (#10652) 27c1333 LF: Drop outdated TODOs (#10725) 9be577a Enable pruning in the sandbox-classic when the append-only schema is used [DPP-567] (#10708) 9f072ae Ledger-API Conformance test for Contract ID V0 (#10717) 867547c DPP-368 enable append-only flag in sandbox (#10710) bdc511e [DPP-438] Change open-ended metric names into static ones (by removing partyName part) (#10706) 0c82006 [Divulgence pruning] Prune all divulged contracts only after migration offset [DPP-483] (#10661) 2555dbb Use soft references for values in the caches (#10715) 7fd5906 Add LedgerClientWithoutLedgerId next to the LedgerClient (#10681) 856c69c participant-integration-api: Increase a test timeout. (#10721) b86d07d remove non functioning oracle json array indices (#10720) f5e1756 sandbox-classic: Remove default parameters in `SqlLedger.Owner`. (#10718) 9ef3377 LF: Update specification with Contract ID Comparability check (#10703) e5c4734 enable JSON search index on Oracle by default (#10539) 1ded42f [DPP-418] Protect TLS keys - follow-up cleanup (#10696) 3fcd986 Introduce a new `Offset` format [KVL-1063] (#10668) a5781a6 update NOTICES file (#10714) 8985505 participant-integration-api: Use deadlines, not deduplication times, for expiring trackers. [KVL-1009] (#10704) 65025c2 sandbox-classic: Add ErrorInfo metadata for rejections. [KVL-1048] (#10707) 97bda3c LF: V1 Contract ID check in Preprocessor (#10687) c2f90ef Add CLI option to force disabling of participant deduplication (#10698) 6016633 Construct ParsedModule directly in Daml Repl (#10701) bbdf16a DPP-368 unhide append-only CLI flags (#10697) a41b134 Use the tracker retention period as the maximum expiry time. [KVL-1009] (#10700) e750ba5 Make warning less scary. (#10699) 5f120bd rotate release duty after 1.17.0-snapshot.20210824.7647.0.640fb683 (#10660) a17253f DPP-535 Verify postgres version (#10577) 301ce53 participant-integration-api: Add tests for ApiCommandService. [KVL-1009] (#10689) bd01a21 [DPP-418] Protect Participant TLS keys (#10629) 7ee1324 update NOTICES file (#10695) 7c392f3 update NOTICES file (#10693) 3db654e update NOTICES file (#10690) eff09c0 ledger-api-client: Wrap command submissions in a new class. [KVL-1009] (#10683) d54adb2 Ledger-API: Conformance tests for contract IDs suffixing (#10654) aa2e869 [Divulgence pruning] Pass divulged contract arguments through kvutils Write/ReadService [DPP-535] (#10598) 1a78313 Disable DeeplyNestedValueIT suite against canton in Daml repo (#10686) b5f9be3 participant-integration-api: Standardize tracker retention naming. (#10682) 2aa632e ledger-on-sql: Do not increment the dispatcher head on start. (#10684) eabb19d [ledger-api] Add deduplication_duration to deduplication period [kvl-1047] (#10676) 96ad9b5 [Divulgence pruning] All divulgence events pruning [DPP-483] (#10634) 0b7980d Update rules_haskell (#10674) 284edfc Fix FlywayMigrations datasource (#10666) adbe65f Document ActionFail vs CanAbort (#10657) 52e7a6d update compat versions for 1.17.0-snapshot.20210824.7647.0.640fb683 (#10667) f42e6b6 Expose pending contracts in triggers (#10672) 7cc6989 Add multiple ways of specifying deduplication [KVL-1047] (#10601) 53be19f participant-integration-api: Ensure that all waiting, failed, and closed trackers are cleaned up. (#10662) b27cde6 participant-integration-api: Move tracker code around, and tidy up tests. (#10663) fc9d359 Drop alternative rules from dlint config (#10646) 5204d3c Include committers in PartialTransaction root context (#10665) 387c68b Normalize transaction values within the engine (#10648) 430c1cc release 1.17.0-snapshot.20210824.7647.0.640fb683 (#10659) ef239fd participant-integration-api: Move `TrackerMap` code around. [KVL-1009] (#10653) ``` Changelog: ``` - [Sandbox] - Participant pruning is enabled in the sandbox-classic when the append-only schema is used - [Ledger Client Scala Bindings] A new variant of the LedgerClient class was added called `LedgerClientWithoutLedgerId`. This class does not need a ledger id at initialization. It was added to allow skipping any checks at initialization for use cases where either the ledger id is not known at initalization or no valid token can be fed at initialization for checking the ledger id. Furthermore for each classes `ActiveContractSetClient`, `CommandClient`, `PackageClient`, `TransactionClient`, `VersionClient` now exists a variant which doesn't depend on a ledger id at initialization and instead requires one for every function as parameter. Moreover the existing classes are extending these classes with overriding the methods and setting the default of the parameter with the given ledger id from initialization. The class `LedgerClientWithoutLedgerId` already makes usage of these variants e.g. `PackageClientWithoutLedgerId`. - [Ledger Client Scala Bindings] The function `transactionSource` of the class `LedgerClientBinding` now optionally accepts a token which is passed on to the unterlying call. - [JSON API] The Oracle database schema has changed; if using ``--query-store-jdbc-config``, you must rebuild the database by adding ``,start-mode=create-only``. See #10539. - [Trigger Service] ``--help`` no longer advertises unsupported JDBC options from JSON API. - [JSON API] [EE only] By default, on Oracle, sets up a JSON search index to speed up the queries endpoints. However, Oracle versions prior to 19.12 have an unrecoverably buggy implementation of this index; in addition, the current implementation fails on queries with strings >256 bytes, with no way to disable the index for that query. Pass the ``disableContractPayloadIndexing=true`` option as part of ``--query-store-jdbc-config`` to disable this index when creating the schema. See `issue #10539 <https://github.com/digital-asset/daml/pull/10539>`__. - [Integration Kit] Changes the Offset format to contain a version and therefore reduces the highest index size by one byte - [Ledger API Server] The command deduplication time is no longer used for determining the period of time to track the command before giving up. Instead, the gRPC deadline is used. If no deadline is provided (or if the deadline exceeds the command tracker retention period), the tracker retention period is used instead. - [Daml Repl] Fix a bug where bindings with out of scope types would result in error in following lines. - [Sandbox, participant] Added a flag to enable a new append-only database schema. This schema was designed to support significantly higher performance. In a future release, all applications will automatically migrate to the new schema. - [Ledger API Server] The command service now uses the tracker retention period (typically specified with the ``--tracker-retention-period`` command-line argument) as the maximum time to wait for a command to arrive on the completion stream. After this time, the command will time out, though it may still complete in the future. Previously, the deduplication period was used, but it was likely the tracker would be terminated before that anyway. The default tracker retention period is 5 minutes, unless otherwise specified. - [DPP-418] [Participant] Add support for supplying server's private key as an encrypted file and then decrypting it with the help of a secrets server. [Integration Kit] KV-based ledgers pass contract instances through the Write/ReadService, removing the need for backfilling divulged contract lookups. Note: KV Ledgers that have been created before this change will still be relying on backfilling lookups of divulged contracts, hence pruning of all divulged contracts may result in failing lookups for divulged contracts. ledger-api - add `deduplication_duration` as a future replacement for `deduplication_time` in the command proto definition ledger-api - Command deduplication period can now be specified by setting `deduplication_offset` instead of `deduplication_time` (only valid for v2 WriteService). This change is backwards compatible. ``` CHANGELOG_BEGIN CHANGELOG_END
aherrmann-da
pushed a commit
that referenced
this pull request
Sep 1, 2021
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. @aherrmann-da is in charge of this release. Commit log: ``` f058c2f DPP-368 clean up flags (#10711) 90ad24f [Divulgence pruning] Prune immediate divulgence [DPP-513] (#10691) 183934b Command dedup: add columns to completions in append-only schemas [KVL-1057] (#10652) 27c1333 LF: Drop outdated TODOs (#10725) 9be577a Enable pruning in the sandbox-classic when the append-only schema is used [DPP-567] (#10708) 9f072ae Ledger-API Conformance test for Contract ID V0 (#10717) 867547c DPP-368 enable append-only flag in sandbox (#10710) bdc511e [DPP-438] Change open-ended metric names into static ones (by removing partyName part) (#10706) 0c82006 [Divulgence pruning] Prune all divulged contracts only after migration offset [DPP-483] (#10661) 2555dbb Use soft references for values in the caches (#10715) 7fd5906 Add LedgerClientWithoutLedgerId next to the LedgerClient (#10681) 856c69c participant-integration-api: Increase a test timeout. (#10721) b86d07d remove non functioning oracle json array indices (#10720) f5e1756 sandbox-classic: Remove default parameters in `SqlLedger.Owner`. (#10718) 9ef3377 LF: Update specification with Contract ID Comparability check (#10703) e5c4734 enable JSON search index on Oracle by default (#10539) 1ded42f [DPP-418] Protect TLS keys - follow-up cleanup (#10696) 3fcd986 Introduce a new `Offset` format [KVL-1063] (#10668) a5781a6 update NOTICES file (#10714) 8985505 participant-integration-api: Use deadlines, not deduplication times, for expiring trackers. [KVL-1009] (#10704) 65025c2 sandbox-classic: Add ErrorInfo metadata for rejections. [KVL-1048] (#10707) 97bda3c LF: V1 Contract ID check in Preprocessor (#10687) c2f90ef Add CLI option to force disabling of participant deduplication (#10698) 6016633 Construct ParsedModule directly in Daml Repl (#10701) bbdf16a DPP-368 unhide append-only CLI flags (#10697) a41b134 Use the tracker retention period as the maximum expiry time. [KVL-1009] (#10700) e750ba5 Make warning less scary. (#10699) 5f120bd rotate release duty after 1.17.0-snapshot.20210824.7647.0.640fb683 (#10660) a17253f DPP-535 Verify postgres version (#10577) 301ce53 participant-integration-api: Add tests for ApiCommandService. [KVL-1009] (#10689) bd01a21 [DPP-418] Protect Participant TLS keys (#10629) 7ee1324 update NOTICES file (#10695) 7c392f3 update NOTICES file (#10693) 3db654e update NOTICES file (#10690) eff09c0 ledger-api-client: Wrap command submissions in a new class. [KVL-1009] (#10683) d54adb2 Ledger-API: Conformance tests for contract IDs suffixing (#10654) aa2e869 [Divulgence pruning] Pass divulged contract arguments through kvutils Write/ReadService [DPP-535] (#10598) 1a78313 Disable DeeplyNestedValueIT suite against canton in Daml repo (#10686) b5f9be3 participant-integration-api: Standardize tracker retention naming. (#10682) 2aa632e ledger-on-sql: Do not increment the dispatcher head on start. (#10684) eabb19d [ledger-api] Add deduplication_duration to deduplication period [kvl-1047] (#10676) 96ad9b5 [Divulgence pruning] All divulgence events pruning [DPP-483] (#10634) 0b7980d Update rules_haskell (#10674) 284edfc Fix FlywayMigrations datasource (#10666) adbe65f Document ActionFail vs CanAbort (#10657) 52e7a6d update compat versions for 1.17.0-snapshot.20210824.7647.0.640fb683 (#10667) f42e6b6 Expose pending contracts in triggers (#10672) 7cc6989 Add multiple ways of specifying deduplication [KVL-1047] (#10601) 53be19f participant-integration-api: Ensure that all waiting, failed, and closed trackers are cleaned up. (#10662) b27cde6 participant-integration-api: Move tracker code around, and tidy up tests. (#10663) fc9d359 Drop alternative rules from dlint config (#10646) 5204d3c Include committers in PartialTransaction root context (#10665) 387c68b Normalize transaction values within the engine (#10648) 430c1cc release 1.17.0-snapshot.20210824.7647.0.640fb683 (#10659) ef239fd participant-integration-api: Move `TrackerMap` code around. [KVL-1009] (#10653) ``` Changelog: ``` - [Sandbox] - Participant pruning is enabled in the sandbox-classic when the append-only schema is used - [Ledger Client Scala Bindings] A new variant of the LedgerClient class was added called `LedgerClientWithoutLedgerId`. This class does not need a ledger id at initialization. It was added to allow skipping any checks at initialization for use cases where either the ledger id is not known at initalization or no valid token can be fed at initialization for checking the ledger id. Furthermore for each classes `ActiveContractSetClient`, `CommandClient`, `PackageClient`, `TransactionClient`, `VersionClient` now exists a variant which doesn't depend on a ledger id at initialization and instead requires one for every function as parameter. Moreover the existing classes are extending these classes with overriding the methods and setting the default of the parameter with the given ledger id from initialization. The class `LedgerClientWithoutLedgerId` already makes usage of these variants e.g. `PackageClientWithoutLedgerId`. - [Ledger Client Scala Bindings] The function `transactionSource` of the class `LedgerClientBinding` now optionally accepts a token which is passed on to the unterlying call. - [JSON API] The Oracle database schema has changed; if using ``--query-store-jdbc-config``, you must rebuild the database by adding ``,start-mode=create-only``. See #10539. - [Trigger Service] ``--help`` no longer advertises unsupported JDBC options from JSON API. - [JSON API] [EE only] By default, on Oracle, sets up a JSON search index to speed up the queries endpoints. However, Oracle versions prior to 19.12 have an unrecoverably buggy implementation of this index; in addition, the current implementation fails on queries with strings >256 bytes, with no way to disable the index for that query. Pass the ``disableContractPayloadIndexing=true`` option as part of ``--query-store-jdbc-config`` to disable this index when creating the schema. See `issue #10539 <https://github.com/digital-asset/daml/pull/10539>`__. - [Integration Kit] Changes the Offset format to contain a version and therefore reduces the highest index size by one byte - [Ledger API Server] The command deduplication time is no longer used for determining the period of time to track the command before giving up. Instead, the gRPC deadline is used. If no deadline is provided (or if the deadline exceeds the command tracker retention period), the tracker retention period is used instead. - [Daml Repl] Fix a bug where bindings with out of scope types would result in error in following lines. - [Sandbox, participant] Added a flag to enable a new append-only database schema. This schema was designed to support significantly higher performance. In a future release, all applications will automatically migrate to the new schema. - [Ledger API Server] The command service now uses the tracker retention period (typically specified with the ``--tracker-retention-period`` command-line argument) as the maximum time to wait for a command to arrive on the completion stream. After this time, the command will time out, though it may still complete in the future. Previously, the deduplication period was used, but it was likely the tracker would be terminated before that anyway. The default tracker retention period is 5 minutes, unless otherwise specified. - [DPP-418] [Participant] Add support for supplying server's private key as an encrypted file and then decrypting it with the help of a secrets server. [Integration Kit] KV-based ledgers pass contract instances through the Write/ReadService, removing the need for backfilling divulged contract lookups. Note: KV Ledgers that have been created before this change will still be relying on backfilling lookups of divulged contracts, hence pruning of all divulged contracts may result in failing lookups for divulged contracts. ledger-api - add `deduplication_duration` as a future replacement for `deduplication_time` in the command proto definition ledger-api - Command deduplication period can now be specified by setting `deduplication_offset` instead of `deduplication_time` (only valid for v2 WriteService). This change is backwards compatible. ``` CHANGELOG_BEGIN CHANGELOG_END Co-authored-by: Azure Pipelines DAML Build <support@digitalasset.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes possible to reject V0 contract IDs during preprocessing.
CHANGELOG_BEGIN
CHANGELOG_END
Pull Request Checklist
CHANGELOG_BEGINandCHANGELOG_ENDtagsNOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with
/AzurePipelines runtotrigger the build.