Skip to content

Cip 0105 with custom templates POC - #6894

Closed
jonored wants to merge 17 commits into
canton-network:staging-0.8.0from
obsidiansystems:cip-0105-custom-templates
Closed

Cip 0105 with custom templates POC#6894
jonored wants to merge 17 commits into
canton-network:staging-0.8.0from
obsidiansystems:cip-0105-custom-templates

Conversation

@jonored

@jonored jonored commented Aug 20, 2026

Copy link
Copy Markdown

Implement governance locks with separate templates and not on top of V2 allocations (but do provide the V2 allocation interface for the separate lock templates).

Next steps are to do a proper happy path test and then add a sketch of the token standard V1 interface according to the implementation CIP work-in-progress.

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).

@meiersi-da meiersi-da left a comment

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.

Nice! Good work and great to see how much simpler the code became with our decision to separate the implementation concerns. Thanks for driving this PoC!

Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
changeHolding

-- TODO: implement V2 AllocationFactory in terms of above.
-- TODO: find and use binding point for "magic address" V1 token standard to above.

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.

It's here:

amulet_transferFactoryV1_transferImpl
: ExternalPartyAmuletRules
-> [Party]
-> Api.Token.TransferInstructionV1.TransferFactory_Transfer
-> Update Api.Token.TransferInstructionV1.TransferInstructionResult
amulet_transferFactoryV1_transferImpl this actors arg = do

Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
signatory dso, owner
choice GovernanceLock_Unlock : GovernanceLock_UnlockResult
with
amount : Optional Decimal

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.

I'm not sure it is worth making this optional. In particular when storing the locked amount on the template it is very easy for the caller to select the full amount themselves if they want to do so.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Counterargument: both of the planned main public interfaces to this choice use a "withdraw from this agreement entirely" primitive that does not inherently supply an amount for a partial withdrawal, so an optional amount here avoids a bit of duplication. On the other hand, we could disallow V2 withdrawal without an amount in the metadata, which would make V1 and V2 handling differ enough to invalidate that argument.

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.

I would aim for V2 to be as easy to use as the compatibility mode. So it should support the default. I'm OK either way wrt the defaulting.

I realize that my main concern is about the implementation: there I'd just compute the concrete amount to unlock up front, and make the code work with that.

require "Only the authorizer may withdraw" $ authorizers == [owner] -- TODO: custom controllers
currentAmount <- fetchAmount (ForOwner with dso; owner) holding
assertDeadlineExceeded "requestAt must be in the past for withdraw" requestedAt
require "withdrawal time must be after the start time" $ requestedAt > startTime

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.

for polish: switch to require' from the TokenStandardUtils, which gives better error messages out of the box. Example usage here:

require' ("inputAmount", inputAmount) isGreaterOrEqualR ("minimal outputAmount", outputFundingAmount)

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.

There actually seems to be a simpler option available: just compute the funds available for withdrawal at requestedAt given the state of the VestingLock and forbid withdraw in case there are no funds available.

This takes care in a uniform way of delayed unlocks, repeated withdrawals, and the final withdrawal.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Final withdraw probably branches away from this code entirely, as it can just leave the LockedAmulet in place; agreed with regard to just checking that there's anything to withdraw.

Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml

lockAmulet : ForOwner -> [Decimal] -> [ContractId Holding] -> ExternalPartyTransferContext -> Update ([ContractId LockedAmulet], Optional (ContractId Amulet))
lockAmulet forOwner@ForOwner {..} amounts inputHoldingCids context = do
let lockContext = "governance"

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.

polish: encode type of lock here, and whether it is vesting

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Still WIP; addressing tomorrow, this was a little messier in the helpers than I had time for today.

Co-authored-by: Simon Meier <simon@digitalasset.com>
Signed-off-by: Jonathan D.K. Gibbons <jonored@gmail.com>
Co-authored-by: Simon Meier <simon@digitalasset.com>
Signed-off-by: Jonathan D.K. Gibbons <jonored@gmail.com>
Co-authored-by: Simon Meier <simon@digitalasset.com>
Signed-off-by: Jonathan D.K. Gibbons <jonored@gmail.com>
Comment thread daml/splice-amulet-test/daml/Splice/Scripts/TestGovernanceLocks.daml Outdated
And one fix for a non-nonconsuming choice.
This probably doesn't matter in actual use, but it _does_ cause more
numbers in the test suite to yield obviously correct round numeric results,
which is useful to make correctness obvious, and does not seem to have
significantly impaired the readability of the implementation.
Comment thread daml/splice-amulet-test/daml/Splice/Scripts/TestGovernanceLocks.daml Outdated
Comment thread daml/splice-amulet-test/daml/Splice/Scripts/TestGovernanceLocks.daml Outdated
Comment thread daml/splice-amulet-test/daml/Splice/Scripts/TestGovernanceLocks.daml Outdated
Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
require "Only the authorizer may withdraw" $ authorizers == [owner] -- TODO: custom controllers
currentAmount <- fetchAmount (ForOwner with dso; owner) holding
assertDeadlineExceeded "requestAt must be in the past for withdraw" requestedAt
require "withdrawal time must be after the start time" $ requestedAt > startTime

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.

There actually seems to be a simpler option available: just compute the funds available for withdrawal at requestedAt given the state of the VestingLock and forbid withdraw in case there are no funds available.

This takes care in a uniform way of delayed unlocks, repeated withdrawals, and the final withdrawal.

Comment on lines +154 to +164
unlockResult <- exercise holding LockedAmulet_UnlockV2
pure VestingLock_WithdrawResult with
vestingLock = None
unlocked = unlockResult.amuletCid
Some partial -> do -- We have a fraction that is not the whole being withdrawn, do a partial unlock.
(locked, unlocked) <- partialUnlock (ForOwner with dso; owner) holding partial context
vestingLock <- Some <$> create this with
holding = locked
pure VestingLock_WithdrawResult with
vestingLock
unlocked

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.

if you implement splitLock such that it returns Optional (ContractId LockedAmulet) for the remainder, then you can just use that here as well; and you get a simpler implementation of _Unlock as well.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I tried to get that to be simpler but I'm not really happy with it; what we need here is the change from executeExternalPartyTransfer and the amount we aren't relocking, and what we need for Unlock is a new LockedAmulet with the same info that it creates from a TransferOutput and the remainder after the new allocation we create; I didn't see any particularly elegant or straightforward way to merge those paths much more than they already are in the lockAmulet helper.

I think the complete withdrawal branch of this choice actually ends up equivalent to (and probably with a shared implementation with) the Expire choice once the holding has the shorter deadline, anyways; I don't see a reason to disturb it if it's directly usable.

Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml
Co-authored-by: Simon Meier <simon@digitalasset.com>
Signed-off-by: Jonathan D.K. Gibbons <jonathan.gibbons@obsidian.systems>
Signed-off-by: Jonathan D.K. Gibbons <jonathan.gibbons@obsidian.systems>
Signed-off-by: Jonathan D.K. Gibbons <jonathan.gibbons@obsidian.systems>
Signed-off-by: Jonathan D.K. Gibbons <jonathan.gibbons@obsidian.systems>
Signed-off-by: Jonathan D.K. Gibbons <jonathan.gibbons@obsidian.systems>
subject : GovernanceLockSubject
deriving (Eq, Show, Serializable)

-- FIXME: move the choices of this factory into `ExternalPartyAmuletRules` once we're happy with them

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.

let's do this as part of polishing this PR

Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml
Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml
Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
Comment thread daml/splice-amulet/daml/Splice/GovernanceLocks.daml Outdated
Co-authored-by: Simon Meier <simon@digitalasset.com>
Signed-off-by: Jonathan D.K. Gibbons <jonathan.gibbons@obsidian.systems>
Co-authored-by: Simon Meier <simon@digitalasset.com>
Signed-off-by: Jonathan D.K. Gibbons <jonathan.gibbons@obsidian.systems>
Signed-off-by: Jonathan D.K. Gibbons <jonathan.gibbons@obsidian.systems>
…e to make it allowed.

Signed-off-by: Jonathan D.K. Gibbons <jonathan.gibbons@obsidian.systems>
Signed-off-by: Jonathan D.K. Gibbons <jonathan.gibbons@obsidian.systems>
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.

7 participants