fix: add confirmation dialog to claimUnstakedTrx method#236
fix: add confirmation dialog to claimUnstakedTrx method#236ulissesferreira wants to merge 1 commit intomainfrom
claimUnstakedTrx method#236Conversation
ulissesferreira
force-pushed
the
fix/no-confirmation-for-withdrawing-unstaked-trx
branch
from
dafbd19 to
d790592
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
ulissesferreira
force-pushed
the
fix/no-confirmation-for-withdrawing-unstaked-trx
branch
from
d790592 to
b62f5bf
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
mikesposito
left a comment
mikesposito
left a comment
There was a problem hiding this comment.
Looks good overall! I just have a couple of questions / small suggestions
| }); | ||
|
|
||
| if (!confirmed) { | ||
| throw new UserRejectedRequestError() as unknown as Error; |
There was a problem hiding this comment.
UserRejectedRequestError is not an error class. I don't know if it was supposed to be used with new Error(new UserRejectedRequestError()), but the class variables and methods seem to be very similar to what Error has, so maybe there will be no issue.
In that case, we could try without as unknown:
| throw new UserRejectedRequestError() as unknown as Error; | |
| throw new UserRejectedRequestError() as Error; |
There was a problem hiding this comment.
Weirdly, the last time I tried this we had some errors, I think linting ones. In the meantime all is good... Well, done ✅
| throw new Error(`Unhandled keyring request method: ${method}`); | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Is this removal intentional?
There was a problem hiding this comment.
Yeah. It was dead code due to the existance of a default case. I guess some historical artifact left behind
| try { | ||
| preferences = await this.#snapClient.getPreferences(); | ||
| } catch { | ||
| // keep defaults |
There was a problem hiding this comment.
Curious: is this expectable in a scenario where the snap is working properly?
There was a problem hiding this comment.
That is a very good question... It falls under those cases where yeah, it should never happen and if it does then the whole snap environment is acting weird. We don't really have an approach / way to handle this. When Snaps were first shipped we were worried about breaking main functionality and this kind of error never got thrown. Do you think we should turn these cases into a panic so we look into them?
There was a problem hiding this comment.
We should not let the entire snap crash, though in cases of undefined behavior like this one it might be good to return an error to the snap request.
Not sure if it is useful for this case, but I see that the #handleClaimUnstakedTrx returns this object:
return {
valid: true,
errors: [],
};
Perhaps this would be a good case to return valid: false and an error in the errors array? We should make sure that it would bubble up to the client UI in the way we would expect, though. What do you think?
ulissesferreira
force-pushed
the
fix/no-confirmation-for-withdrawing-unstaked-trx
branch
from
b62f5bf to
dfec0ee
Compare
2 participants
Explanation
Added a confirmation dialog before signing and broadcasting the claim of unstaked TRX, using the
ConfirmSignTransactionUI component. The dialog estimates fees and displays relevant asset information to the user. If the user rejects, aUserRejectedRequestErroris thrownScreen.Recording.2026-03-12.at.13.47.10.mov
References
n/a
Checklist