WIP Get started on topup allocations - #12
Conversation
…cation metadata for governance allocation arguments
| isGovernanceAllocationArgument arg = not ( S.null $ governanceMetadataKeys arg.allocation.meta ) | ||
| || not (TM.null ( TM.filterWithKey ( \a _ -> "cip-105/" `T.isPrefixOf` a ) arg.extraArgs.context.values)) |
There was a problem hiding this comment.
@jonored @dbzguy56 : what do you think about simplifying this logic as follows:
- determine whether this is a governance allocation call based on a single metadata key
cip-105/operation-typethat encodes the type of operation. - forbid setting any
cip-105/keys in case that is not set
This way we follow standard practices for encoding sum types, and can provide better error messages.
There was a problem hiding this comment.
I don't particularly have any objection to inverting isGovernanceAllocationArgument into a check on cip-105/operation-type and then this check inverted for non-governance-related allocations. Once we enter the actual metadata conversion I think we do follow the usual practice in our typeclass, we just didn't when adding the loose functions for context keys.
I actually think that we might want to do a bit of matching on the choice context separately as well as parse the metadata; if we accept a topup and a governance lock spec together at the daml layer (and check that the topup matches the provided lock spec), then the http API can handle mapping allocation calls into topup operations when possible by finding a matching existing allocation to pass in the context. It looks like it can't modify the metadata as well to do that.
There was a problem hiding this comment.
I don't particularly have any objection to inverting isGovernanceAllocationArgument into a check on cip-105/operation-type and then this check inverted for non-governance-related allocations. Once we enter the actual metadata conversion I think we do follow the usual practice in our typeclass, we just didn't when adding the loose functions for context keys.
OK. Let's convert to a principled way then.
I actually think that we might want to do a bit of matching on the choice context separately as well as parse the metadata; if we accept a topup and a governance lock spec together at the daml layer (and check that the topup matches the provided lock spec), then the http API can handle mapping allocation calls into topup operations when possible by finding a matching existing allocation to pass in the context. It looks like it can't modify the metadata as well to do that.
I don't understand what you mean: where do you want to do the matching?
| None -> do | ||
| let eitherCid = lookupFromContext @(ContractId V2.Allocation) alloc.extraArgs.context topUpAllocationMetadataKey |
There was a problem hiding this comment.
See my comment above. This approach of try parsing it as a lock, then as a top-up, is prone to surprises. Let's just encode the type of operation directly.
| authorizer = alloc.allocation.authorizer | ||
| transferLegSides = [] | ||
| settlementDeadline = Some endDate -- Might as well let it unlock normally at the end of vesting. | ||
| settlementDeadline = Some maxComparableTime |
There was a problem hiding this comment.
It'll change back to this as soon as we modify the allocation with settlement as it is now; we need to use this max time for the LockedAmulet contract and we're currently doing that by setting it in computeAllocationExpiry if there's a governance lock. We could do a change to split the values where we'd need to to make a None settlementDeadline on a governance lock mean actually no deadline and the lock persists rather than the maxTTL-but-resettable deadline behavior normal allocations have, but I'm not convinced of the value.
There was a problem hiding this comment.
I was also looking at this and noticed that you can't withdraw from a committed allocation with this deadline set to None, and that's presumably the reason.
| vestingLockCid = toInterfaceContractId @V2.Allocation aCid | ||
|
|
||
| now <- getFromContextU arg.extraArgs.context effectiveAtKey -- FIXME: read from metadata, not context. | ||
| isLedgerTimeGE now >>= require "effectiveAt must not be in the future" |
There was a problem hiding this comment.
calling this effectiveAt feels off. I'd suggest to encode a requestedAt argument in our virtual choice argument (GovernanceLockOperation_TopUp) and parse it normally.
Also you can just use assertDeadlineExceeded "requestedAt" requestedAt to encode your restriction.
4 participants
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