Skip to content

[ci] Use the actual Splice.Amulet.CryptoHash functions in CryptoHashProxy#5118

Open
adetokunbo wants to merge 1 commit intoadetokunbo/cip-104-add-lookup-open-mining-round-by-numberfrom
adetokunbo/use-actual-crypto-hash-in-equivalence-test
Open

[ci] Use the actual Splice.Amulet.CryptoHash functions in CryptoHashProxy#5118
adetokunbo wants to merge 1 commit intoadetokunbo/cip-104-add-lookup-open-mining-round-by-numberfrom
adetokunbo/use-actual-crypto-hash-in-equivalence-test

Conversation

@adetokunbo
Copy link
Copy Markdown
Contributor

Fixes #4749

Pull Request Checklist

Cluster Testing

  • If a cluster test is required, comment /cluster_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.
  • If a hard-migration test is required (from the latest release), comment /hdm_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.
  • If a logical synchronizer upgrade test is required (from canton-3.5), comment /lsu_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.

PR Guidelines

  • Include any change that might be observable by our partners or affect their deployment in the release notes.
  • Specify fixed issues with Fixes #n, and mention issues worked on using #n
  • Include a screenshot for frontend-related PRs - see README or use your favorite screenshot tool

Merge Guidelines

  • Make the git commit message look sensible when squash-merging on GitHub (most likely: just copy your PR description).

@adetokunbo adetokunbo added this to the Traffic-Based App Rewards milestone Apr 20, 2026
@adetokunbo adetokunbo self-assigned this Apr 20, 2026
do pure (hashVariant tag fields)
do pure (hashVariant tag (map Hash fields)).value

nonconsuming choice CryptoHashProxy_HashMintingAllowance : Text
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't these be modified to work with actual data?

data MintingAllowance = MintingAllowance with
    provider : Party
    amount : Decimal

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I looked into this but it doesn't work cleanly for the equivalence test.

The issue is that the Daml Hashable instances for Party and
Decimal apply their own serialization (partyToText, show) before
hashing, and these don't necessarily match the text strings we pass to
the SQL side. For example, show on Decimal(0) produces
"0.0000000000" not "0", and party IDs go through partyToText.

The purpose of this test is to verify the hash algorithm is equivalent
(Daml == SQL), not to test Daml's type serialization. Keeping the proxy
choices as Text args lets us control the exact string representation
on both sides, which is what we need for the equivalence comparison.

The proxy still uses the real Splice.Amulet.CryptoHash primitives
(hash, hashRecord, hashVariant) — it's only the domain composites
(hashMintingAllowance, batch hashes) that stay as text-based wrappers
to avoid the serialization mismatch.

Copy link
Copy Markdown
Contributor

@dfordivam dfordivam Apr 21, 2026

Choose a reason for hiding this comment

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

Ultimately we have to make sure that hash on Batch matches what we compute in sql.
Can we show that this is the case?
If there are any serialization mismatches here, then we have an issue on the SQL/scala side.

    choice ProcessRewardsV2_ProcessBatch : ProcessRewardsV2_ProcessBatchResult
      with
        batch : Batch
        providersWithWrongVettingState : Set Party
          -- ^ Service providers that do not have the correct vetting state for receiving rewards.
      observer batchProviders providersWithWrongVettingState batch
      controller dso
      do
        let actualHash = CryptoHash.hash batch
        require "batch hash matches" (actualHash == batchHash)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you could say that it is not feasible to match the daml serialization in scala/sql. In that case we must change the instance of Batch in daml. But that will require further discussion with Simon

Copy link
Copy Markdown
Contributor

@dfordivam dfordivam left a comment

Choose a reason for hiding this comment

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

Discussed with Tim that we will keep this PR open for now, and create a separate PR for adding equivalence testing of Batch hash with the actual APIs in store doing the computation of hashes from activity-records. And because the new PR would also modify the daml and will require all the .dar, etc updates, we should keep this one open and merge both together.

confirming that CryptoHashEquivalenceIntegrationTest still succeeds

Signed-off-by: Tim Emiola <adetokunbo@emio.la>
@adetokunbo adetokunbo force-pushed the adetokunbo/use-actual-crypto-hash-in-equivalence-test branch from 156298e to 8621670 Compare April 23, 2026 10:29
@adetokunbo adetokunbo changed the base branch from dfordivam/cip-104-sv-app-rewards to adetokunbo/cip-104-add-lookup-open-mining-round-by-number April 23, 2026 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants