Top-up allocations Take 2 by dbzguy56 · Pull Request #14 · obsidiansystems/splice

dbzguy56 · 2026-08-24T21:03:10Z

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 an upgrade test is required, comment /upgrade_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).

cgibbard · 2026-08-25T15:50:27Z

+        unlockedAmuletCid <- unlockAmulet (ForOwner with dso; owner) holding
+        unlockedAmulet <- fetchChecked @Amulet (ForOwner with dso; owner) unlockedAmuletCid
+
+        -- TODO: Should change holding here always be none? Like if we want to topup,


Either we should assert that it's None, if that's indeed the case, or it should be part of the result of the operation probably.

jonored · 2026-08-25T15:45:51Z

+        unlockedAmulet <- fetchChecked @Amulet (ForOwner with dso; owner) unlockedAmuletCid
+
+        -- TODO: Should change holding here always be none? Like if we want to topup,
+        -- we want to do the full amount we're asked of?


I expect that change can be non-None here, they don't have to make a new Amulet just to control how much they top up by. It's not change relative to the amount we're asked, it's the change from breaking down the input holdings into the amount we're adding to the lock and the change we get back.

jonored · 2026-08-25T15:50:38Z

+
+        -- TODO: Should change holding here always be none? Like if we want to topup,
+        -- we want to do the full amount we're asked of?
+        ([newHolding], changeHolding) <- lockAmulet


I'd like this to be a helper below for "add this amount from these holdings to this LockedAmulet", not just unlockAmulet. It can take the amount to increase as well, rather than the amount of the new lock.

I'd be inclined to make it have "cannot net unlock Amulet" as a checked invariant as well, but we could also plausibly relax those invariants and merge it and partialUnlock.

I think I have what was asked here in the first paragraph. I'm a little lost on the second, but I can ask you about that tomorrow.

jonored · 2026-08-25T15:52:21Z

      governanceLock : Optional (ContractId GovernanceLock)
  deriving (Eq, Show, Serializable)

+data GovernanceLock_TopUpResult


This should also have the change output, I think that enables chaining a few operations together with their "change" outputs and it's just sensible to report the new holdings.

jonored · 2026-08-25T15:53:10Z

+        actors : [Party]
+      controller owner
+      do
+        require "Only the owner may Top Up on an GovernanceLock" $ actors == [owner] -- TODO: custom controllers


Definitely also need to know that the amount is positive, so this isn't also "unlock without vesting".

meiersi-da

Thanks. Looks like straight top-ups are simple. Thanks for adding the test and catching the bug in your implementation.

meiersi-da · 2026-08-26T06:16:07Z

+  inputs <- fmap fst <$> queryInterface @Holding owner
+  (Some governanceLock) <- queryContractId @GovernanceLock owner cid
+  -- We want to make sure that we don't include the governanceLock holding as an input
+  let inputs' : [ContractId Holding] = deleteBy (==) (toInterfaceContractId @Holding governanceLock.holding) inputs


use listUnlockedHoldingCidsFor (basicAccount owner) owner env.amuletId where https://github.com/canton-network/splice/blob/1ee0f00917d8f741bd6f33779bebcdaef5bcbc84/token-standard/splice-token-standard-v2-test/daml/Splice/Testing/TokenStandard/WalletClientV2.daml#L190-L194 and I assume that you define amuletId in your TestEnv. The AmuletRegistryV2 has it directly: https://github.com/canton-network/splice/blob/1ee0f00917d8f741bd6f33779bebcdaef5bcbc84/token-standard/splice-token-standard-v2-test/daml/Splice/Testing/Registries/AmuletRegistryV2.daml#L79-L82

meiersi-da · 2026-08-26T06:17:34Z

+        actors : [Party]
+      controller owner


This doesn't make sense: don't accept actors when you set a fixed controller.

Suggested change

actors : [Party]

controller owner

controller owner

meiersi-da · 2026-08-26T06:19:08Z

+        actors : [Party]
+      controller owner
+      do
+        require "Only the owner may Top Up on a GovernanceLock" $ actors == [owner] -- TODO: custom controllers


Suggested change

require "Only the owner may Top Up on a GovernanceLock" $ actors == [owner] -- TODO: custom controllers

meiersi-da · 2026-08-26T06:27:11Z

+  unlockedAmulet <- fetchChecked @Amulet forOwner unlockedAmuletCid
+  lockAmulet
+    forOwner
+    [unlockedAmulet.amount.initialAmount + amount]


@jonored : I remember you wanted to change GovernanceLock to store its own amount directly, correct? That should also be pulled through to the code here.

This is true, I have now merged in the latest code from Jonathan's branch

meiersi-da · 2026-08-26T06:28:59Z


+    choice GovernanceLock_TopUp : GovernanceLock_TopUpResult
+      with
+        amount : Decimal


Suggested change

amount : Decimal

topupAmount : Decimal -- ^ Amount by which the locked amount should be increased.

meiersi-da

Thanks. Looks good to me assuming you address the suggestions I've added.

meiersi-da · 2026-08-27T06:58:11Z

  pure holdingView.amount
+
+addAmountToLockedAmulet : ContractId LockedAmulet -> ForOwner -> ExternalPartyTransferContext -> [ContractId Holding] -> Decimal -> Update ([ContractId LockedAmulet], Optional (ContractId Amulet))
+addAmountToLockedAmulet lockedAmulet forOwner context inputHoldingCids amount = do


Suggested change

addAmountToLockedAmulet lockedAmulet forOwner context inputHoldingCids amount = do

changeLockedAmuletAmount lockedAmulet forOwner context inputHoldingCids amount = do

as you are not doing an add, but a change.

meiersi-da · 2026-08-27T06:59:02Z

+
+        ([newHolding], changeHolding) <- addAmountToLockedAmulet lockedAmulet (ForOwner with dso; owner) context inputs (amount + topupAmount)


Suggested change

([newHolding], changeHolding) <- addAmountToLockedAmulet lockedAmulet (ForOwner with dso; owner) context inputs (amount + topupAmount)

let newAmount = amount + topupAmount

([newHolding], changeHolding) <- addAmountToLockedAmulet lockedAmulet (ForOwner with dso; owner) context inputs newAmount

this way you can reuse it below, and are guaranteed the amounts are in sync

Initial top-up allocations commit ontop of cip-0105-custom-templates

7ba2ab0

dbzguy56 changed the title ~~Initial top-up allocations commit ontop of cip-0105-custom-templates~~ Top-up allocations v2 Aug 25, 2026

cgibbard reviewed Aug 25, 2026

View reviewed changes

jonored reviewed Aug 25, 2026

View reviewed changes

Add top up test and apply comment suggestions

19cff7b

meiersi-da reviewed Aug 26, 2026

View reviewed changes

dbzguy56 changed the title ~~Top-up allocations v2~~ Top-up allocations Take 2 Aug 26, 2026

dbzguy56 added 4 commits August 26, 2026 18:31

Apply latest topup review suggestions

8247924

Merge branch 'cip-0105-custom-templates' into cip-0105-topup-allocs-v2

03f0235

Shuffle addAmountToLockedAmulet arithmetic around

0bf19ff

Remove a comment

5dc10eb

meiersi-da approved these changes Aug 27, 2026

View reviewed changes

Fix topup lock amulet function naming

92d9680

	amount : Decimal
	topupAmount : Decimal -- ^ Amount by which the locked amount should be increased.

	addAmountToLockedAmulet lockedAmulet forOwner context inputHoldingCids amount = do
	changeLockedAmuletAmount lockedAmulet forOwner context inputHoldingCids amount = do


		([newHolding], changeHolding) <- addAmountToLockedAmulet lockedAmulet (ForOwner with dso; owner) context inputs (amount + topupAmount)

-        ([newHolding], changeHolding) <- addAmountToLockedAmulet lockedAmulet (ForOwner with dso; owner) context inputs (amount + topupAmount)
+        let newAmount = amount + topupAmount
+        ([newHolding], changeHolding) <- addAmountToLockedAmulet lockedAmulet (ForOwner with dso; owner) context inputs newAmount

Uh oh!

Conversation

dbzguy56 commented Aug 24, 2026

Pull Request Checklist

Cluster Testing

PR Guidelines

Merge Guidelines

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meiersi-da left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meiersi-da left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants