Add DB versions of Splice.Amulet.CryptoHash#4662
Add DB versions of Splice.Amulet.CryptoHash#4662adetokunbo wants to merge 6 commits intoadetokunbo/cip-104-add-compute-reward-totalsfrom
Conversation
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
| 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.
...in/resources/db/migration/canton-network/postgres/stable/V065__app_reward_hash_functions.sql
Outdated
Show resolved
Hide resolved
...in/resources/db/migration/canton-network/postgres/stable/V065__app_reward_hash_functions.sql
Outdated
Show resolved
Hide resolved
...in/resources/db/migration/canton-network/postgres/stable/V065__app_reward_hash_functions.sql
Outdated
Show resolved
Hide resolved
We can and should test the equivalence to the Daml implementation. I'd suggest to do this as follows:
|
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
63dcc69 to
56f1b77
Compare
77ce31d to
05e30d8
Compare
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>
|
@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)
|
|
||
| def toSqlExpr(op: HashOp): String = op match { | ||
| case HashText(v) => | ||
| s"daml_crypto_hash_text('${escapeSql(v)}')" |
There was a problem hiding this comment.
For a test like this is good enough. For production code, you can avoid all unsafe interpolation and manual quoting using
(
sql"select " ++ sql"daml_crypto_hash_text($v)" ++ ...
).toActionBuilder| // Re-add TestEngine so Splice tests can run Daml code in-memory | ||
| Test / unmanagedSources := Seq( | ||
| (Test / sourceDirectory).value / "scala/com/digitalasset/canton/util/TestEngine.scala" | ||
| ), |
There was a problem hiding this comment.
I'm wary of this dependency, as it is only available because we fork Canton.
I'd much prefer to just use the normal participant nodes we have available, and thereby be safe both from divergences in the behavior as well as be safe from having to be able to provision this dependency.
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