Feat/fix: wait for tx to resolve/reject using resolversMap

[zenodeapp]

Hi there!

So I'm currently working on the IBC shielded transfer app, which communicates with the Namada extension and stumbled upon the following problem:

Sent transactions to the extension immediately resolve/reject, which make it impossible for the browser to wait for a result and decide whether the tx failed or not.

This PR solves this and also optimizes/fixes/restructures/refactors some of the resolver logic; see more technical details below.

With this fix the frontend is capable of awaiting sent transactions and also able to see whether the user approves or declines the transaction (or closes the window). Declining makes the browser aware of the promises' rejection with an optional message field, while a successful transaction resolves with optional additional details.

ZEN

[zenodeapp]

Changelog

• Feature/fix: transaction-functions in service.ts now leverage the resolverMap to prevent premature resolves/rejections of promises.
• Optimization: remove resolvers after use (garbage collection, but also to prevent accidentally resolving or rejecting multiple times).
• Refactoring: abstracted logic for creating, resolving and rejecting promises.
• Feature: Generic type parameter added for the createResolver-function. This will especially be useful for when type assertions for all possible transactions get implemented.
• Feature: Arbitrary error-handling messages added to rejections, defaulting to a "Request rejected"-message.
• Bugfix: small bugfix in the error thrown in the approveConnectionResponse-function.
• Bugfix: implemented a logic where signatures/transactions get rejected whenever the user closes the popup prematurely. This did require attaching logic to the beforeunload-event of the window. See Appendix for more info.

To-do list

• Reject promise if someone closes the popup prematurely.
• Fix resolving the promise once the transaction is submitted successfully. I realized that my transaction was missing the PublicKey in the txArgs which caused the extension to panic and never resolve. I opened a new PR as it's somewhat outside the scope of this issue (see fix: make publicKey a required field in the TxMsgValue class. #659).
• (Possibly add a timeout? If there's no response?). Likely not needed anymore, see previous point.

Appendix

• The functions for transactions + signatures are starting to look a lot like one another, so perhaps the logic could be refactored/abstracted even further. Though, then again, this might not be what is desired, so be careful not to create a scenario where these two become too intertwined (to adhere to loose coupling/tight cohesion). If I already entered this territory, please tell me!
• The logic for rejecting signatures/transactions when a user closes the popup manually did force me to introduce a global variable in the utils-file named: performUnloadCleanup. I did this to be able to distinguish between closing the window programmatically or manually; else a submitted transaction that called closeCurrentTab would instantly tell the browser the pending promise got rejected. Now whenever the popup gets closed via code the performUnloadCleanup variable gets toggled off, the closing of the tab gets performed without performing any rejections, followed by defaulting the performUnloadCleanup back to true.
• Issue fix: make publicKey a required field in the TxMsgValue class. #659 introduces a new problem where transactions panic at the extension-level (probably in wasm code already?). This causes the promises to not fulfill. These errors should probably get send back to the browser to trigger a rejection of sorts. At the moment, I do not immediately see how to solve this.
• I may have not fixed the unit test-file correctly (also: missing describe("submitTx")?).

[zenodeapp]

Alright, pooh! Think this now has most of the logic I wanted to implement. It's sometimes difficult to navigate inside other people's code 😅! Hope I didn't make a mess. Tell me if I did!

To explain this PR in simple terms:

• This PR is only for the wallet's extension code.
• The frontend can now wait for a transaction's completion. Visually you could for instance create a loading screen while it awaits the promised result.
• Error handling/messages could therefore be returned for better feedback to the user and give a more streamlined experience.
• Prematurely closing the popup triggers a rejection of the signature or transaction, allowing the promise to become determinate.
• I attempted to refactor the logic (especially the resolver) and take care of duplicate code. Though do tell me if I made it too tightly coupled.

[emccorson]

Thanks for the contribution and detailed explanation! I should hopefully be able to review this next week if no one else gets there first.

One thing to mention is that it is already possible to know if a transaction has completed, because the extension fires an event ("namada-tx-completed", I think). But I'll think about how this compares to being able to get a response from the Promise.

[zenodeapp]

Thanks for the contribution and detailed explanation! I should hopefully be able to review this next week if no one else gets there first.

One thing to mention is that it is already possible to know if a transaction has completed, because the extension fires an event ("namada-tx-completed", I think). But I'll think about how this compares to being able to get a response from the Promise.

Ah interesting, didn't know this! That's also indeed a way to go at it! Take your time and no pressure/biggy if you decide this won't make the cut :)!

UPDATE: Due to the thought process below, I may need to rephrase this PR. It's more so about knowing whether a tx we initiated got submitted correctly or not. It might also be a good thing to consider the direction or options given to the front-end developer: do we listen to every tx event fired or do we selectively wait for the result of a transaction we initiated?

But I actually think it's possible to have both ways implemented without loss of performance or anything! Plus it's consistent that way, considering signatures already had promises! (unless it was planned to remove this!)

[zenodeapp]

Just a small post-refactoring I'll do. I think it's better to separate the logic of creating a window/popup and attaching a resolver to it. Will commit this after getting out of bed 😴.

[zenodeapp]

Just a small post-refactoring I'll do. I think it's better to separate the logic of creating a window/popup and attaching a resolver to it. Will commit this after getting out of bed 😴.

Alright done.

• I was contemplating to call: isResolver => pending or unresolved since it actually is an unresolved promise if it's still present in the resolverMap. Though kept it as isResolver.
• Also, I wonder if it's actually needed to do this isResolver-check at the top of submitTx, submitSignature, rejectSignature and rejectTx functions. Last commit I re-introduced it back in the rejectSignature and rejectTx functions as I accidentally removed this extra check previously. The only reason to include it is to be able to get the error thrown: no resolvers found for tab ID ${popupTabId} if for some reason the tabId isn't available in the resolversMap anymore. If we don't include this check then it 'fails' in silence since the resolve and reject functions already check whether they can fulfill the promise or not.

But yes, just a couple of contemplations. In my opinion the check seems a bit overkill, could perhaps remove the isResolver function in its entirety. Don't think it causes any troubles if I go through the code. But I might be wrong!

PS: It might actually be necessary to not include this check if I think of this TODO comment: //TODO: Shouldn't we _clearPendingSignature when throwing?. So maybe it is necessary to remove the whole isResolver check as to not cause any loose pendingSignature- or pendingTx-data to dangle around. But tell me if this check is obsolete, then I'll push a new commit removing this in its entirety. (Only if this PR gets merged of course).

[zenodeapp]

Just a small post-refactoring I'll do. I think it's better to separate the logic of creating a window/popup and attaching a resolver to it. Will commit this after getting out of bed 😴.

Alright done.

• I was contemplating to call: isResolver => pending or unresolved since it actually is an unresolved promise if it's still present in the resolverMap. Though kept it as isResolver.
• Also, I wonder if it's actually needed to do this isResolver-check at the top of submitTx, submitSignature, rejectSignature and rejectTx functions. Last commit I re-introduced it back in the rejectSignature and rejectTx functions as I accidentally removed this extra check previously. The only reason to include it is to be able to get the error thrown: no resolvers found for tab ID ${popupTabId} if for some reason the tabId isn't available in the resolversMap anymore. If we don't include this check then it 'fails' in silence since the resolve and reject functions already check whether they can fulfill the promise or not.

But yes, just a couple of contemplations. In my opinion the check seems a bit overkill, could perhaps remove the isResolver function in its entirety. Don't think it causes any troubles if I go through the code. But I might be wrong!

PS: It might actually be necessary to not include this check if I think of this TODO comment: //TODO: Shouldn't we _clearPendingSignature when throwing?. So maybe it is necessary to remove the whole isResolver check as to not cause any loose pendingSignature- or pendingTx-data to dangle around. But tell me if this check is obsolete, then I'll push a new commit removing this in its entirety. (Only if this PR gets merged of course).

Okay final option lol, which may solve ths whole TODO-issue while still including the error throwing if the tabId isn't around:
Remove the isResolver-check in its entirety but add the thrown error message in both resolve and reject functions as an else-case, like so:

  private resolve(popupTabId: number, result?: unknown) {
    if (popupTabId in this.resolverMap) {
      this.resolverMap[popupTabId].resolve(result);
      delete this.resolverMap[popupTabId];
    } else {
      throw new Error(`no resolvers found for tab ID ${popupTabId}`);
    }
  }

  private reject(popupTabId: number, message?: any) {
    if (popupTabId in this.resolverMap) {
      this.resolverMap[popupTabId].reject(message ?? new Error("Request rejected"));
      delete this.resolverMap[popupTabId];
    } else {
      throw new Error(`no resolvers found for tab ID ${popupTabId}`);
    }
  }

For this to work, it does require the resolving or rejecting to always happen AFTER clearing pending data..

Choices, choices 😅, I hope the bombardment of details doesn't make ya go nuts.

[zenodeapp]

Merged latest into this PR. Luckily already fixed the things that got pushed to main.