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
Fix transaction encoding
  • Loading branch information
@oggy-
oggy- committed Jan 15, 2020
commit 8fa86b29253f338c79336642d74fc8a3b26b9b4e
7 changes: 2 additions & 5 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 @@ -199,15 +199,12 @@ object TransactionCoder {
case Some(KeyWithMaintainers(_, _))
if transactionVersion precedes minContractKeyInExercise =>
Right(())
case Some(KeyWithMaintainers(_, maintainers))
if maintainers.isEmpty && !(transactionVersion precedes minMaintainersInExercise) =>
Left(EncodeError(
s"Key maintainers in exercise nodes must be non-empty for transactions of version $transactionVersion"))
case Some(KeyWithMaintainers(k, maintainers)) =>
encodeVal(k).map { encodedKey =>
exBuilder.setContractKey(encodedKey._2)
}
exBuilder.addAllKeyMaintainers(maintainers.toSet[String].asJava)
if(!(transactionVersion precedes minMaintainersInExercise))
exBuilder.addAllKeyMaintainers(maintainers.toSet[String].asJava)
Right(())
Copy link
Collaborator

Choose a reason for hiding this comment

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

[Cosmetic]
I found this pattern matching with booleans a bit confusing.
I woul prefer something like

  _ <- e.key match {
            case Some(KeyWithMaintainers(k, maintainers))
                if !(transactionVersion precedes minContractKeyInExercise) =>
              if ((transactionVersion precedes minMaintainersInExercise) && maintainers.nonEmpty)
                Left(EncodeError(transactionVersion, isTooOldFor = "maintainers in NodeExercises"))
              else
                encodeVal(k).map { encodedKey =>
                  exBuilder.setContractKey(encodedKey._2)
                  exBuilder.addAllKeyMaintainers(maintainers.toSet[String].asJava)
                  ()
                }
            case _ =>
              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.

I also messed up the logic here. I also changed the style now but I'm ambivalent to that.

}
} yield nodeBuilder.setExercise(exBuilder).build()
Expand Down
7 changes: 7 additions & 0 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 @@ -301,6 +301,11 @@ class TransactionCoderSpec
case _ => gn
}

def withoutMaintainersInExercise[Nid, Cid, Val](gn: GenNode[Nid, Cid, Val]): GenNode[Nid, Cid, Val] =
gn match {
case ne: NodeExercises[Nid, Cid, Val] => ne copy (key = ne.key.map(_.copy(maintainers = Set.empty)))
case _ => gn
}
def transactionWithout[Nid: Ordering, Cid, Val](
t: GenTransaction[Nid, Cid, Val],
f: GenNode[Nid, Cid, Val] => GenNode[Nid, Cid, Val]): GenTransaction[Nid, Cid, Val] =
Expand All @@ -315,6 +320,8 @@ class TransactionCoderSpec
(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))
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to do withoutMaintainersInExercise in both prior if branches (see how withoutContractKeyInExercise is also done in the branch prior its precedence test), or (ideally) reorganize this function so the precedence tests are independent of each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added and reorganized. Less efficient when no changes need to be done, but this is test code.

else tx

}