Add DB versions of Splice.Amulet.CryptoHash#4662
Conversation
| object DbCryptoHashFunctionsTest { | ||
|
|
||
| /** Scala reimplementation of Daml's CryptoHash module (Splice.Amulet.CryptoHash). | ||
| * Used as the test oracle to verify plpgsql hash functions produce identical output. |
There was a problem hiding this comment.
Please also add a corresponding test that calls the actual Daml functions.
I'd do so by adding a Splice.Amulet.TestTemplates module defining a template CryptoHashProxy that allows evaluating calls to crypto hashes.
There was a problem hiding this comment.
rautenrieth-da
left a comment
There was a problem hiding this comment.
Looks good, I'm only slightly nervous about the test strategy. We are comparing the SQL output to a re-implementation of the daml code in scala, with only two hardcoded hashes manually copied from the daml output. I think it's still fine because we don't expect changes in #3964, but I'd like to hear input on this from @meiersi-da.
Unfortunately I don't think it's possible to execute arbitrary daml functions from scala code. We have an in-memory daml engine (com.digitalasset.canton.util.TestEngine) but that takes a command and produces a transaction.
We can and should test the equivalence to the Daml implementation. I'd suggest to do this as follows:
|
63dcc69 to
56f1b77
Compare
77ce31d to
05e30d8
Compare
|
@rautenrieth-da , @meiersi-da all comments addressed, PTAL |
rautenrieth-da
left a comment
There was a problem hiding this comment.
Thanks, test coverage looks good now!
(don't forget to add the signed-off-by to the last commit)
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
- the new test does three-way testing between db, daml and a scala oracle Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
e8f64ee to
112f172
Compare
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
- no need for dependencies on Canton Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
…eded Signed-off-by: Tim Emiola <adetokunbo@emio.la>
…est uses Signed-off-by: Tim Emiola <adetokunbo@emio.la>
43cc969 to
c32cf53
Compare
rautenrieth-da
left a comment
There was a problem hiding this comment.
Nice job removing the dependency on the daml engine!
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
f090592
into
adetokunbo/feature-cip-104-scan-computation
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Merges the feature branch for the scan computation part of CIP-104, consisting of the following individual PRs: * Add app activity computation store and schema (#4372) * Compute aggregateActivityTotals, add ComputeAppRewardTrigger, expose new endpoints (#4420) * Adds computeRewardTotals to DbScanAppRewardsStore (#4639) * Add DB versions of Splice.Amulet.CryptoHash (#4662) * Expand GetRewardAccountingActivityTotalsResponse (#4859) * Add counters to RewardComputationTrigger (#4860) * Compute Hashes (#4847) Signed-off-by: Tim Emiola <adetokunbo@emio.la> Signed-off-by: Timothy Emiola <adetokunbo@users.noreply.github.com> Co-authored-by: Simon Meier <simon@digitalasset.com> Co-authored-by: Robert Autenrieth <31539813+rautenrieth-da@users.noreply.github.com>
First part the changes that address #4382
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./lsu_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines