Cip 0105 with custom templates POC - #6894
Conversation
meiersi-da
left a comment
There was a problem hiding this comment.
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!
| changeHolding | ||
|
|
||
| -- TODO: implement V2 AllocationFactory in terms of above. | ||
| -- TODO: find and use binding point for "magic address" V1 token standard to above. |
There was a problem hiding this comment.
It's here:
splice/daml/splice-amulet/daml/Splice/ExternalPartyAmuletRules.daml
Lines 341 to 346 in 3e4bb9a
| signatory dso, owner | ||
| choice GovernanceLock_Unlock : GovernanceLock_UnlockResult | ||
| with | ||
| amount : Optional Decimal |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
for polish: switch to require' from the TokenStandardUtils, which gives better error messages out of the box. Example usage here:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| lockAmulet : ForOwner -> [Decimal] -> [ContractId Holding] -> ExternalPartyTransferContext -> Update ([ContractId LockedAmulet], Optional (ContractId Amulet)) | ||
| lockAmulet forOwner@ForOwner {..} amounts inputHoldingCids context = do | ||
| let lockContext = "governance" |
There was a problem hiding this comment.
polish: encode type of lock here, and whether it is vesting
There was a problem hiding this comment.
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>
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.
ea6c7be to
d584c7c
Compare
| 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 |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
34e743d to
a04de0a
Compare
Co-authored-by: Simon Meier <simon@digitalasset.com> Signed-off-by: Jonathan D.K. Gibbons <jonathan.gibbons@obsidian.systems>
a04de0a to
1514a92
Compare
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 |
There was a problem hiding this comment.
let's do this as part of polishing this PR
Co-authored-by: Simon Meier <simon@digitalasset.com> Signed-off-by: Jonathan D.K. Gibbons <jonathan.gibbons@obsidian.systems>
7f4655f to
1d29830
Compare
Co-authored-by: Simon Meier <simon@digitalasset.com> Signed-off-by: Jonathan D.K. Gibbons <jonathan.gibbons@obsidian.systems>
0f26d35 to
59b4db2
Compare
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>
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
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./upgrade_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