-
Notifications
You must be signed in to change notification settings - Fork 206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LF Provide function to reversion a contract #19738
base: main
Are you sure you want to change the base?
Conversation
translateValue(typ, value).map(_.toNormalizedValue(dstVer)) | ||
|
||
// This | ||
def reversion( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've experimented with using this API directly, and as @matthiasS-da observed, the problem we have is that we may observe several Nodes
that bind the contract to different target template ids. As I expect we want to use the one with the maximum package version. Then we also need a method an API that calls this converter with the
we only populate with view with a single contract instance although the transaction tree:
def reversion(
contract: FatContractInstance,
maxVersionOfTmplId: Set[Ref.TypeConName],
): Result[FatContractInstance]
) | ||
case None => ResultNone | ||
} | ||
} yield contract.toImplementation.copy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously that argument was consistent with the templateId so it is not nice this is no longer the case. It also means that (contract.templateId == dstTmplId) => ResultDone(contract)
will not work for pre-converted contracts. An alternative could be to also template the templateId
with the converted templateId. If this is to be considered Hash.assertHashContractInstance
would need to be modified not to include the packageId
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add an optional reversionedPackageId
to FatContractInstance
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@remyhaemmerle-da Please read @simonmaxen-da 's first comment and double-check there is no fundamental problem with the engine. That said, it is totally up to you how you handle things inside of the engine.
Maybe add an optional reversionedPackageId to FatContractInstance ?
Please keep in mind that such a change may be challenging to consume in the Canton repo. If you add the field, please take care of the fall-out, too.
contract: FatContractInstance, | ||
dstTmplId: Ref.TypeConName, | ||
): Result[FatContractInstance] = { | ||
if (contract.templateId == dstTmplId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would not work for pre-converted contracts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@remyhaemmerle-da please double-check there is no problem with the engine. That said, up to you how you handle things.
@simonmaxen-da and myself have reorganized ourselves. As a result, I'll take over the review of this PR. |
My main critique right now is that the interface here does not (yet) match what we have agreed on. For convenience, let me copy over the signature:
With the following constraints:
Additionally: Last but not least: please double-check that the |
No description provided.