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

Prohibit contract IDs in contract keys and add key maintainers to exercises #4048

Merged
merged 14 commits into from
Jan 20, 2020
Merged

Prohibit contract IDs in contract keys and add key maintainers to exercises #4048

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1d1f228
Prohibit contract IDs in contract keys and add key maintainers to exe…
oggy- Jan 13, 2020
f05f20f
Apply suggestions from code review
oggy- Jan 15, 2020
baac6bc
Update daml-lf/transaction/src/main/scala/com/digitalasset/daml/lf/tr…
oggy- Jan 15, 2020
62e33ee
Address remy's comments + fix some suggested changes
oggy- Jan 15, 2020
3b78600
scalafmt
oggy- Jan 15, 2020
8fa86b2
Fix transaction encoding
oggy- Jan 15, 2020
2e037a6
scalafmt
oggy- Jan 15, 2020
b4024ad
Stephen's comments
oggy- Jan 16, 2020
042f27d
Address the backwards incompatibility in the appropriate sections of …
oggy- Jan 16, 2020
7347c49
Merge branch 'master' into add-key-maintainers-to-exercises
oggy- Jan 16, 2020
d90360c
Apply suggestions from code review
oggy- Jan 16, 2020
342c158
Adjust KV utils to the .proto change
oggy- Jan 16, 2020
5180d55
Merge branch 'add-key-maintainers-to-exercises' of github.com:digital…
oggy- Jan 16, 2020
dc2b942
Treat as a bugfix
oggy- Jan 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Stephen's comments
  • Loading branch information
@oggy-
oggy- committed Jan 16, 2020
commit b4024ad5480a90574e42f9cd5f90e506e79bca3d
12 changes: 10 additions & 2 deletions daml-lf/spec/transaction.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,12 @@ In this version, these fields are included:

``maintainers`` must be non-empty.

*since version 9*
oggy- marked this conversation as resolved.
Show resolved Hide resolved

The key may not contain contract IDs. This change is backwards
incompatible; that is, transactions containing contract IDs in keys
cannot be deserialized any more.
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in a previous comment

it would be improper to only mention this restriction in a since version 9 section in transaction.rst (as commented elsewhere), since you are imposing the restriction retroactively.

That's because you can always read the spec such that if you are dealing with version n, you can ignore all sections since version e where e > n; that is not the case here.

Effectively, the restriction you are introducing is since version 3, in other words, on all KeyWithMaintainers.


message NodeCreate
^^^^^^^^^^^^^^^^^^

Expand Down Expand Up @@ -507,8 +513,10 @@ contract has a contract key defined.

*since version 9*

New optional field `key_maintainers` is now set when the exercised
contract has a contract key defined.
New optional field `key_with_maintainers` is now set when the exercised
contract has a contract key defined. The `contract_key` field is
not used any more.


message NodeLookupByKey
^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
2 changes: 1 addition & 1 deletion daml-lf/transaction/src/main/protobuf/com/digitalasset/daml/lf/transaction.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ message NodeExercise {
com.digitalasset.daml.lf.value.ContractId contract_id_struct = 11;
com.digitalasset.daml.lf.value.VersionedValue return_value = 12;
com.digitalasset.daml.lf.value.VersionedValue contract_key = 13; // optional
repeated string key_maintainers = 14; // optional
KeyWithMaintainers key_with_maintainers = 14; // optional
}

message NodeLookupByKey {
Expand Down
52 changes: 31 additions & 21 deletions ...lf/transaction/src/main/scala/com/digitalasset/daml/lf/transaction/TransactionCoder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,19 +194,22 @@ object TransactionCoder {
s"Trying to encode transaction of version $transactionVersion, which requires the exercise return value, but did not get exercise return value in node."))
case (_, true) => Right(())
}
_ <- e.key match {
case None => Right(())
case Some(KeyWithMaintainers(_, _))
if transactionVersion precedes minContractKeyInExercise =>
Right(())
case Some(KeyWithMaintainers(k, maintainers)) =>
encodeVal(k).map { encodedKey =>
exBuilder.setContractKey(encodedKey._2)
_ <- Right(
e.key
.map { kWithM =>
if (transactionVersion precedes minContractKeyInExercise) ()
else if (transactionVersion precedes minMaintainersInExercise) {
encodeVal(kWithM.key).map { encodedKey =>
exBuilder.setContractKey(encodedKey._2)
}
} else
encodeKeyWithMaintainers(encodeVal, kWithM).map {
case (_, encodedKey) =>
exBuilder.setKeyWithMaintainers(encodedKey)
}
()
}
if (!(transactionVersion precedes minMaintainersInExercise))
exBuilder.addAllKeyMaintainers(maintainers.toSet[String].asJava)
Right(())
}
.getOrElse(()))
} yield nodeBuilder.setExercise(exBuilder).build()

case nlbk: NodeLookupByKey[Cid, Val] =>
Expand Down Expand Up @@ -315,18 +318,25 @@ object TransactionCoder {
Left(DecodeError(txVersion, isTooOldFor = "exercise result"))
else Right(None)
} else decodeVal(protoExe.getReturnValue).map(Some(_))
contractKey <- if (protoExe.hasContractKey) {
hasKeyWithMaintainersField = (protoExe.getKeyWithMaintainers != TransactionOuterClass.KeyWithMaintainers.getDefaultInstance)
keyWithMaintainers <- if (protoExe.hasContractKey) {
if (txVersion precedes minContractKeyInExercise)
Left(DecodeError(txVersion, isTooOldFor = "contract key in exercise"))
else if (!(txVersion precedes minMaintainersInExercise))
Left(DecodeError(
s"contract key field in exercise must not be present for transactions of version $txVersion"))
else if (hasKeyWithMaintainersField)
Left(DecodeError(
"an exercise may not contain both contract key and contract key with maintainers"))
else
decodeVal(protoExe.getContractKey).map(Some(_))
decodeVal(protoExe.getContractKey).map(k => Some(KeyWithMaintainers(k, Set.empty)))
} else if (hasKeyWithMaintainersField) {
if (txVersion precedes minMaintainersInExercise)
Left(DecodeError(txVersion, isTooOldFor = "NodeExercises maintainers"))
else
decodeKeyWithMaintainers(decodeVal, protoExe.getKeyWithMaintainers).map(k => Some(k))
} else Right(None)
maintainers <- toPartySet(protoExe.getKeyMaintainersList)
_ <- Either.cond(
!(txVersion precedes minMaintainersInExercise) || maintainers.isEmpty,
(),
DecodeError(txVersion, isTooOldFor = "NodeExercises maintainers")
)

oggy- marked this conversation as resolved.
Show resolved Hide resolved
ni <- nodeId
targetCoid <- protoExe.decodeContractIdOrStruct(decodeCid, txVersion)(
_.getContractId,
Expand Down Expand Up @@ -364,7 +374,7 @@ object TransactionCoder {
controllers = controllers,
children = children,
exerciseResult = rv,
key = contractKey.map(k => KeyWithMaintainers(k, maintainers))
key = keyWithMaintainers
))
case NodeTypeCase.LOOKUP_BY_KEY =>
val protoLookupByKey = protoNode.getLookupByKey
Expand Down
22 changes: 12 additions & 10 deletions ...ransaction/src/test/scala/com/digitalasset/daml/lf/transaction/TransactionCoderSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,15 +315,17 @@ class TransactionCoderSpec

def minimalistTx[Nid: Ordering, Cid, Val](
txvMin: TransactionVersion,
tx: GenTransaction[Nid, Cid, Val]): GenTransaction[Nid, Cid, Val] =
if (txvMin precedes minExerciseResult)
transactionWithout(
tx,
(x: GenNode[Nid, Cid, Val]) => withoutExerciseResult(withoutContractKeyInExercise(x)))
else if (txvMin precedes minContractKeyInExercise)
transactionWithout(tx, (x: GenNode[Nid, Cid, Val]) => withoutContractKeyInExercise(x))
else if (txvMin precedes minMaintainersInExercise)
transactionWithout(tx, (x: GenNode[Nid, Cid, Val]) => withoutMaintainersInExercise(x))
else tx
tx: GenTransaction[Nid, Cid, Val]): GenTransaction[Nid, Cid, Val] = {
def condApply(before: TransactionVersion, f: GenNode[Nid, Cid, Val] => GenNode[Nid, Cid, Val])
: GenNode[Nid, Cid, Val] => GenNode[Nid, Cid, Val] =
if (txvMin precedes before) f else identity

transactionWithout(
tx,
condApply(minMaintainersInExercise, withoutMaintainersInExercise)
.compose(condApply(minContractKeyInExercise, withoutContractKeyInExercise))
.compose(condApply(minExerciseResult, withoutExerciseResult))
)
}

}