feat: registerErrorTransformer in composer #371
Conversation
Co-authored-by: Tristan Menzel <[email protected]>
Co-authored-by: Tristan Menzel <[email protected]>
|
The problem is that since this is an error originating from an HTTP call we have no control over how granular the context we inject can be (i.e. we could inject group or the txid we waited for, but not per-transaction level details without also parsing the error) nor do we have control over the error itself. I don't think |
Co-authored-by: Tristan Menzel <[email protected]>
joe-p
force-pushed
the
feat/composer_error_callbacks
branch
from
d390b5c to
b96eea6
Compare
|
@joe-p do you consider this new functionality applicable to python utils as well? Given that the py version is up to speed with ts counterpart with v3 (which is in feedback cycle stage at the moment), makes sense to keep feature parity in mind when extending interfaces on either py or ts side. |
|
Ah yes good point @aorumbayev. I could either PR into your branch next week or to reduce moving parts we can merge your PR first and then have a separate PR for this functionality. To avoid noise in this issue just DM on slack if you have a preference, otherwise I will make a PR into your branch next week |
| @@ -504,6 +504,7 @@ export class AppClient { | |||
| this._appSpec = AppClient.normaliseAppSpec(params.appSpec) | |||
| this._appName = params.appName ?? this._appSpec.name | |||
| this._algorand = params.algorand | |||
| this._algorand.registerErrorTransformer!(this.handleCallErrors) | |||
There was a problem hiding this comment.
Is the ! necessary here?
There was a problem hiding this comment.
For now, yes. It can be removed next major release when we remove AlgorandClientInterface. See #365 (comment)
src/types/composer.ts
Outdated
| */ | ||
| export type ErrorTransformer = (error: Error) => Promise<Error> | ||
|
|
||
| class NonErrorTransformer extends Error { |
There was a problem hiding this comment.
| class NonErrorTransformer extends Error { | |
| class NonErrorTransformerError extends Error { |
There was a problem hiding this comment.
I've renamed it to something a bit different, InvalidErrorTransformerValue which I think is a bit easier to understand
Unless there is some rush on this, might be more straightforward for you to add this after initial v3 prod release? There is still a bit of tweaks that may happen based on ongoing feedback session with DevRel. Would be significantly easier for you to introduce it once prod release is out and everything is settled. I think this should happen within 1-2 weeks (depending on how long feedback cycle takes), i can ping you as soon as prod version is out. |
Co-authored-by: Tristan Menzel <[email protected]>
Proposed Changes
registerErrorTransformeris a new function on the composer that allow one to register functions that are used to transform errorsThe main benefit is the improved error handling when composing together one group with multiple app clients. It also simplifies the error handling logic in the app client. When an
AppClientis instantiated it registers the transformer to theAlgorandClient. This means errors for that app will always be properly handled, even when the method call did not come from theAppClient. These callbacks are also used on simulate, so this also closes #368.One thing I was going back and forth on is whether these functions should transform the error or if they are expected to raise the error themselves. The main reason I went with transforming is that transformers could add supplement information to the error, which seems useful.
Keeping in draft until #365 is merged since I manually merged that in here