Refactor to parse at allocation creation time. by jonored · Pull Request #8 · obsidiansystems/splice

jonored · 2026-08-05T17:14:39Z

Needs tests updated, they were reusing a few functions and a few of the metadata key names have changed to match the design doc.

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

Needs tests updated, they were reusing a few functions and a few of the metadata key names have changed to match the design doc.

meiersi-da

Thanks. This looks like the right direction.

meiersi-da · 2026-08-05T18:39:40Z


+-- | Key used to provide an effective time to a choice execution, to avoid getTime
+effectiveAtKey : Text
+effectiveAtKey = "effective-at"


Isn't that a metadata key? I'd suggest we scope that to cip-105/

I was feeding it in as a context key, actually - it should be constrained to an appropriately tight bound - did I hear right that the normal one is one day? - so I figured it might as well be part of the context package from the API instead of part of the user-set metadata. I actually checked here first to see whether we already had a conventional key for "now, but with the more loose time bound" and was somewhat surprised not to find one. We can certainly scope it to cip-105, of course.

This value should be chosen by the client not the off-ledger API server. Let's discuss this evening.

meiersi-da · 2026-08-05T18:41:20Z

+    endDate : Time
+    initialAmount : Decimal
+    -- ^ The initial amount represents the total that was locked initially when thrown into vesting state
+    substituteControllers : Optional ControllerSets


don't we also need withdraw controllers? I suspect we also need them on the AggregatedLock.

Discussed, and agreed; the case where we have non-joint auth from apps is clear motivation.

Withdraw on AggregatedLock is controlled by unlockControllers, though. I think the next state is that both are "withdrawControllers".

there might a confusion here:

we are using V2.Allocation_Withdraw for multiple purposes

when unlocking the actors should be constrained by the unlock controllers from https://docs.google.com/document/d/13zl8ILEWq6CvSALk2LA79I2rE1PHfni1EFf5ywr-Aik/edit?tab=t.0#heading=h.imk0o4qckssp

when withdrawing from a vesting lock they must be scoped to the "withdraw" controllers from https://docs.google.com/document/d/13zl8ILEWq6CvSALk2LA79I2rE1PHfni1EFf5ywr-Aik/edit?tab=t.0#heading=h.ddev3dkzc1vd

Needed on the SvLock to pass to the created VestedLock - sorry, I didn't fully remember a bit of the context. Agreed.

meiersi-da · 2026-08-05T18:55:34Z

@@ -1,148 +1,138 @@
 module Splice.AggregateLock where


Suggested change

module Splice.GovernanceLock where

Reasonable. I'm not sure whether this module persists separately or gets folded into AmuletAllocationV2 and ExternalPartyAmuletRules, but having it segregated like this is somewhat nice. A bit of a pain with a few bits being close to circular.

Let's keep at least a separate module for the types and the core functions implementing the logic. If needed, we can then inline the functions that assemble the logic into the modules you mentioned.

That's certainly reasonable. I'm just mildly annoyed at the newEmptyAllocation arguments and generic 'a' argument where this is still relying on specifically AmuletAllocationV2 rules and behavior.

meiersi-da · 2026-08-05T18:56:18Z

+    unlockControllers : Optional ControllerSets
+    substituteControllers : Optional ControllerSets


consider grouping into a GovernanceLockControllers record jointly with withdraw

I'll put that back; the main fuss was trying to use different terminology for SvLock and VestingLock, but really, they can probably both use "withdraw" as a key.

I'd replied without recovering the full context; having a type with unlock, substitute, and withdraw controllers is certainly appropriate for SvLock and not bad for VestingLock.

meiersi-da · 2026-08-05T18:58:11Z

+    substituteControllers : Optional ControllerSets
+  deriving (Eq, Show)
+
+parseGovernanceLock : CanAssert m => Metadata -> m (Optional GovernanceLock)


I would have probably organized this using a type-class FromMetadata a, so that each sub-types parser is defined on its own; and reuse is simplified.

Will do; I usually try to balance direct and local code against type-class and prettier, with smaller amounts leaning towards direct. I'll certainly not say no to splitting it into the type-class.

I usually do the same, but found that for this kind of codec case the overhead on naming and testing becomes significantly lower with a kind of local helper typeclass. For example, this AnyValue codec:

https://github.com/canton-network/splice/blob/df76027a6bdc149184e9ec7911a9978b20c5729e/daml/splice-amulet/daml/Splice/Amulet/TokenApiUtils.daml#L256-L289

meiersi-da · 2026-08-05T19:15:18Z


+
+data GovernanceLock
+  = GovernanceLock_SVLocked AggregatedLock


Suggested change

= GovernanceLock_SVLocked AggregatedLock

= GL_SvLock SvLock

| GL_FeaturedAppLock FeaturedAppLock

| GL_VestingLock VestingLock

meiersi-da · 2026-08-05T19:16:49Z

+  | GovernanceLock_VestingLocked VestingLock
+  deriving (Eq, Show)
+
+data AggregatedLock =


Suggested change

data AggregatedLock =

data SvLock =

meiersi-da · 2026-08-05T19:20:34Z

+data AggregatedLock =
+  AggregatedLock with
+    lockBeneficiary : Text
+    unlockControllers : Optional ControllerSets


Suggested change

unlockControllers : Optional ControllerSets

unlockControllers : Optional ControllerSpecification

-- ^ If set, specifies non-default controllers for unlocking the SV lock.

meiersi-da · 2026-08-07T05:24:54Z

+checkControllerSpecification : TM.TextMap Party -> [Party] -> ControllerSpecification -> Update ()
+checkControllerSpecification svRightsParties actors set =
+    require "Controllers must be one of the listed options" $
+      any actorsMatch set
  where
-    controllerSetFromMetaMap key = fromOptional [[acctParty]] $ controllerSetFromMeta =<< key `TM.lookup` meta.values
+    actorsMatch : [SvLockController] -> Bool
+    actorsMatch set
+      = all (\s -> any (`checkParty` s) actors) set
+      && all (\a -> any (a `checkParty`) set) actors


consider converting to [[Party]] and then using TokenStandardUtils.checkActors

I'll keep checkControllerSpecification as the name and contract as a bit of cheap future-proofing, but no good reason to keep the more complex match if SvLockController always maps one-to-one to parties at an instant; I don't think we see that changing.

Refactor to parse at allocation creation time.

8dc394c

Needs tests updated, they were reusing a few functions and a few of the metadata key names have changed to match the design doc.

meiersi-da reviewed Aug 5, 2026

View reviewed changes

Parse metadata with a typeclass, and add SvLockController sum type.

a290157

meiersi-da reviewed Aug 7, 2026

View reviewed changes

		unlockControllers : Optional ControllerSets
		substituteControllers : Optional ControllerSets

-  = GovernanceLock_SVLocked AggregatedLock
+  = GL_SvLock SvLock
+  | GL_FeaturedAppLock FeaturedAppLock
+  | GL_VestingLock VestingLock

	unlockControllers : Optional ControllerSets
	unlockControllers : Optional ControllerSpecification
	-- ^ If set, specifies non-default controllers for unlocking the SV lock.

Uh oh!

Conversation

jonored commented Aug 5, 2026

Pull Request Checklist

Cluster Testing

PR Guidelines

Merge Guidelines

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!

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!

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 Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

meiersi-da Aug 5, 2026 •

edited

Loading