KV: do not use "Effects" part of InputsAndEffects #9433
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
requested review from
cocreature,
fabiotudone-da,
gerolf-da,
miklos-da and
S11001001
as code owners
6 tasks
remyhaemmerle-da
force-pushed
the
remy-lf-kv-effects
branch
from
f2a4d5c to
2561f7a
Compare
cocreature
force-pushed
the
remy-kv-input
branch
from
6f17c71 to
96f2d82
Compare
|
turning this into a draft, this one needs some more work to be ready for review. |
cocreature
force-pushed
the
remy-lf-kv-effects
branch
from
2561f7a to
b271d89
Compare
cocreature
force-pushed
the
remy-lf-kv-effects
branch
2 times, most recently
from
70e714e to
861c0cc
Compare
The PR is based #9429. changelog_begin changelog_end
cocreature
force-pushed
the
remy-lf-kv-effects
branch
from
861c0cc to
8476a49
Compare
cocreature
requested review from
mziolekda,
nmarton-da,
rautenrieth-da and
tudor-da
as code owners
fabiotudone-da
approved these changes
Apr 20, 2021
Comment on lines
382
to
402
| def create() = { | ||
| val cid = builder.newCid | ||
| val node = builder.create( | ||
| cid, | ||
| template = "-pkg-:Mod:T", | ||
| argument = V.ValueUnit, | ||
| signatories = parties, | ||
| observers = Seq(), | ||
| key = None, | ||
| ) | ||
| (cid, node) | ||
| } | ||
| def exercise(create: Node.NodeCreate[ContractId], consuming: Boolean) = | ||
| builder.exercise( | ||
| contract = create, | ||
| choice = "C", | ||
| actingParties = parties.toSet, | ||
| consuming = consuming, | ||
| argument = V.ValueUnit, | ||
| byKey = false, | ||
| ) |
There was a problem hiding this comment.
Perhaps it's possible to share these utilities between test cases? That would also make them shorter and so more readable.
There was a problem hiding this comment.
Factored out in 60a00a5. Probably could do some more aggressive factoring but I’ll leave that for a future PR.
changelog_begin changelog_end
miklos-da
approved these changes
Apr 20, 2021
daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/Transaction.scala
Outdated
Show resolved
Hide resolved
...ant-state/kvutils/src/main/scala/com/daml/ledger/participant/state/kvutils/Conversions.scala
Show resolved
Hide resolved
| // Add contract state entries to mark contract activeness (checked by 'validateModelConformance'). | ||
| for ((key, createNode) <- effects.createdContracts) { | ||
| localContracts.foreach { case (cid, (nid, createNode)) => |
There was a problem hiding this comment.
Why not use for comprehension for better readability?
Suggested change
| localContracts.foreach { case (cid, (nid, createNode)) => | |
| for((cid, (nid, createNode)) <- localContracts) { |
There was a problem hiding this comment.
Let me expose the original motivations for the change I made here.
- @stephencompall-DA strongly advice to never use the pattern matching in the left-hand side of a
<-inside a for comprehension as it is not translate as you expect. (It think the reason is explained [here](Scala Wart: Convoluted de-sugaring of for-comprehensions scala/scala3#2573, @stephencompall-DA please correct me if I mistake).
If you want to keep using for comprehension, you should rather write something like:
for {
entry <- effects.createdContracts
(key, createNode) = entry
} { ... }
- I prefer avoiding for-comprehension for imperative code.
There was a problem hiding this comment.
Correct, and I wrote a mini-article about it on our forum. And I also agree with your motivation for avoiding the for without yield.
...a/com/daml/ledger/participant/state/kvutils/committer/transaction/TransactionCommitter.scala
Outdated
Show resolved
Hide resolved
Co-authored-by: Miklos <57664299+miklos-da@users.noreply.github.com>
changelog_begin changelog_end
azure-pipelines bot
pushed a commit
that referenced
this pull request
Apr 21, 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. @remyhaemmerle-da is in charge of this release. Commit log: ``` cf2be78 Support rollback nodes in BlindingInfo (#9445) adde90b KV: do not use "Effects" part of InputsAndEffects (#9433) 75fa86a Additional metrics for mutable contract state cache (#9444) d8c34a4 Rename normalization-related params in the integrity checker config (#9455) e335244 Hash submitters in the deduplication key [DPP-347] (#9417) 43ffa42 Test for duplicate contracts when querying on behalf of multiple parties (#9443) 7644844 Document daml ledger export script (#9446) 3193027 DPP-334 Manually partition the event table (#9394) 14661a8 Clearer variable name for subtype evidence (#9442) 40c85d8 Release v1.13.0-snapshot.20210419.6730.0.8c3a8c04 (#9439) 1cb907c KV: do not use "Inputs" part of InputsAndEffects (#9429) ``` Changelog: ``` - [Integration Kit] - deduplication key will contain hashed submitters instead of concatenated submitters * [Daml export] Refer to the "Ledger Export" chapter under the "Early Access Features" for a description of the new Daml ledger export command. This is an early access feature. ``` CHANGELOG_BEGIN CHANGELOG_END
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.
The PR is based one #9429.
Together with #9429 this make InputsAndEffects useless.
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.