UP-18: ledger API split of upgrade errors (Scala changes only) #20350
Conversation
carlpulley-da
commented
Nov 22, 2024
•
carlpulley-da
commented
- Split upgrade errors at the ledger API (Scala only changes)
- Prep Canton side PR (see https://github.com/DACH-NY/canton/pull/22688)
- Prep follow on Daml changes (see UP-18: ledger API split of upgrade errors (Daml-script changes) #20355)
…ript-structured-errors-2.x
…ript-structured-errors-2.x
…ript-structured-errors-2.x
…ript-structured-errors-2.x
| dstTemplateId: Identifier, | ||
| signatories: String, | ||
| observers: String, | ||
| optKey: Option[(String, String)], |
There was a problem hiding this comment.
msg is not captured here as this causes truncation errors
There was a problem hiding this comment.
If we're reaching the limit of grpc error message sizes, I believe theres a way to bump it, perhaps @tudor-da knows?
|
|
||
| @Explanation("An optional contract field with a value of Some may not be dropped during downgrading") | ||
| @Resolution( | ||
| "Ensure optional contract fields with a value of Some are not dropped" |
There was a problem hiding this comment.
We may want to revisit these errors strings, but we'll leave the canton people to give suggestions here
| loggingContext: ContextualizedErrorLogger | ||
| ) extends DamlErrorWithDefiniteAnswer( | ||
| cause = cause | ||
| ) |
There was a problem hiding this comment.
Is there a reason we don't give any info for this message?
There was a problem hiding this comment.
Yes. With my original thinking, both fields would involve converting Value to a string. My current understanding is that this could lead to grpc error headers being truncated, hence why I'd avoided doing this.
Looking again at the contents of Error.Upgrade.DowngradeDropDefinedField, I've realised that the first field has type Ast.Type which might be a reasonable value to serialise.
There was a problem hiding this comment.
Added in changes to capture expectedType
There was a problem hiding this comment.
Is it not possible to simply include the field name?
There was a problem hiding this comment.
I don't think this is currently possible? The current data structure we have to work with is DowngradeDropDefinedField(expectedType: Ast.Type, actualValue: Value) and there's an associated TODO (#17647) that likely is work that would enable doing something like this.
There was a problem hiding this comment.
I think that's worth doing, just extending the error type if its easy.
idm if its this Pr or another
There was a problem hiding this comment.
I plan to do this as part of the work under UP-20 (in addition to handling the lookup errors).
| dstTemplateId: Identifier, | ||
| signatories: String, | ||
| observers: String, | ||
| optKey: Option[(String, String)], |
There was a problem hiding this comment.
If we're reaching the limit of grpc error message sizes, I believe theres a way to bump it, perhaps @tudor-da knows?
