fast-auth-signer icon indicating copy to clipboard operation
fast-auth-signer copied to clipboard

Listing out possible errors from MPC service

Open hcho112 opened this issue 2 years ago • 8 comments

We need MPC to return human readable errors back so that near-discovery can consume and display on toast. Once it is done, we will make following changes on near discovery.

### Tasks
- [ ] List out possible errors from MPC
- [ ] Implement human readable errors on MPC

hcho112 avatar Jul 20 '23 03:07 hcho112

As we discussed, I have pull it out into a ticket and assigned it to you. In my knowledge, making this change won't break existing flow. (otherwise feel free to discuss here)

hcho112 avatar Jul 20 '23 04:07 hcho112

We definitely need to organize all of the possible non-500 errors from mpc-recovery and add them to the public API. What would you prefer this to look like though? Would something like this be fine?

{
    "type": "error",
	"code": 1,
    "msg": "Account ID [email protected] is malformed"
}

or would you prefer something more structured so you can come up with a message on your end e.g.

{
    "type": "error",
	"code": 1,
    "account_id": "[email protected]" // trailing fields are dynamic depending on what error code is, in this case we only need `account_id`
}

CC @volovyks looping you in here in case you want to give your opinion on the matter

itegulov avatar Jul 20 '23 04:07 itegulov

Implement human readable errors on MPC

Also, just to clarify, I think they are reasonably human readable even right now. For example the most common error we return right now is fail: Error: oath_token <token> has already been used to register an account. You can only register 1 account per oath_token, which seems to not be displayed on the frontend side at all.

itegulov avatar Jul 20 '23 04:07 itegulov

We definitely need to organize all of the possible non-500 errors from mpc-recovery and add them to the public API. What would you prefer this to look like though? Would something like this be fine?

{
    "type": "error",
	"code": 1,
    "msg": "Account ID [email protected] is malformed"
}

or would you prefer something more structured so you can come up with a message on your end e.g.

{
    "type": "error",
	"code": 1,
    "account_id": "[email protected]" // trailing fields are dynamic depending on what error code is, in this case we only need `account_id`
}

CC @volovyks looping you in here in case you want to give your opinion on the matter

Personally, I would prefer first approach because it simplifies the logic on receiving side. Unless we are definitely applying translation, we should go for first approach. Thoughts? @MaximusHaximus @DavidM-D

hcho112 avatar Jul 20 '23 05:07 hcho112

Implement human readable errors on MPC

Also, just to clarify, I think they are reasonably human readable even right now. For example the most common error we return right now is fail: Error: oath_token <token> has already been used to register an account. You can only register 1 account per oath_token, which seems to not be displayed on the frontend side at all.

Oh great, didn't know. First of all, is all errors handled like above? Secondly, I think the wording is just fine to me, but to make sure it is more appropriate, these should be passed on to designers + managers to make sure that the wording is friendly enough to general public.

hcho112 avatar Jul 20 '23 05:07 hcho112

Oh great, didn't know. First of all, is all errors handled like above? Secondly, I think the wording is just fine to me, but to make sure it is more appropriate, these should be passed on to designers + managers to make sure that the wording is friendly enough to general public.

Yes, AFAIR we have human readable errors for all possible responses. Some of them might be more cryptic than the example above though.

itegulov avatar Jul 20 '23 06:07 itegulov

@itegulov @hcho112 I think that the first option is fine, but I do not thik that we need "code" field. It's just an unredable duplication of the "type" field. And the type field is human redable BTW. I'm expecting it to be something like "IdTokenValidationFailure". In the end, we will have a finite list of types (strings) that FE can compare against.

{
    "errorType": "BadAccountId",
    "errorMsg": "Account ID [email protected] is malformed: accountId can only consist of numbers..."
}

volovyks avatar Jul 25 '23 07:07 volovyks

@volovyks

By "type": "error" I just meant this is not "type": "ok". For the record, our current deployments return errors like this:

{"type":"err","msg":"failed to verify oidc token: InvalidToken"}

where type is a tag responsible for the enum variants, see this.

Agree with everything else though, we can replace code with something human-readable.

itegulov avatar Jul 25 '23 07:07 itegulov