Feat/70 wallet locked warnings in content scripts API #174
Conversation
|
Visit the preview URL for this PR (updated for commit 51bec06): https://emeris-extension--pr174-feat-70-unlock-warni-1esjoq9m.web.app (expires Mon, 06 Jun 2022 09:15:10 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
caudurodev
force-pushed
the
feat/70-unlock-warnings
branch
3 times, most recently
from
9c5b506 to
6cff6dc
Compare
caudurodev
requested review from
clockworkgr,
luciorubeens,
eitjuh,
pranaybaldev,
Raymahni,
fl-y,
mihailmarcu and
Dawntraoz
as code owners
| 'Wallet not ready for requests. Please create/unlock wallet and whitelist current URL before making requests.', | ||
| ); | ||
| this.enable(); | ||
| return false; |
There was a problem hiding this comment.
why return false here? can't we just proceed after the user accepts the page?
There was a problem hiding this comment.
No, because the user needs to generally needs to unlock / create their wallet, then enable the website, then can make requests. This is the simplified approach where the popup needs to be opened each time a request is made and the next step is requested from the user.
I tried to keep the running queue of possible requests in memory, but it ended up too complicated and bad UX. For example:
- User requests signing, but has no wallet. Requests needed to proceed:
- create a wallet, store mnemonic, backup later, etc (can terminate interaction in different places)
- whitelist website
- finally proceed with signing
- User requests signing, has wallet but is locked. Requests needed to proceed:
- enter wallet password
- whitelist website
- finally proceed with signing
etc.
I attempted to queue these requests but it just became very complex as I had to detect the extension closing and checking the queue for more requests and then re-opening it. I think this initial approach is simpler for now. But we can discuss how to keep the queue and keep requesting user interaction as it's quite complex.
There was a problem hiding this comment.
why does the user need to create a wallet to enable a website?
There was a problem hiding this comment.
what do you mean by queue requests? where? can we not just await this.enable()?
There was a problem hiding this comment.
why does the user need to create a wallet to enable a website?
Because it would be a security issue to whitelist without first unlocking your extension. Or is your question about differentiating account creation from wallet creation? Can you create an account without a wallet?
what do you mean by queue requests? where? can we not just await this.enable()?
because this would cause all requests to queue in the current implementation. Each time you made a request the queue would increase until finally unlocked.
There was a problem hiding this comment.
Can you create an account without a wallet?
you can set a password without creating a wallet.
There was a problem hiding this comment.
the requests don't reach the Emeris.ts queue as they are send after the showPopupIfNotEnabled function. or am I missunderstanding sth? and keeping the couple of requests in the JS memory is not a problem is it?
There was a problem hiding this comment.
Might be simpler to discuss in a call @faboweb ?
| @@ -109,6 +128,7 @@ export class ProxyEmeris implements IEmeris { | |||
| return response.data as boolean; | |||
| } | |||
| async supportedChains(): Promise<string[]> { | |||
| if (!(await this.showPopupIfNotEnabled())) return; | |||
There was a problem hiding this comment.
this now also fails when the user doesn't have an account created yet. does that make sense?
There was a problem hiding this comment.
Not sure - Like I mentioned in the PR description I don't know what all methods do so if this needs to be unblocked let me know as it's easy to just remove.
There was a problem hiding this comment.
I've removed the block for now.
There was a problem hiding this comment.
as the supported chains are likely also encrypted, I guess we still need to check if a password is set and the wallet is unlocked
There was a problem hiding this comment.
Ok, blocked it again.
caudurodev
force-pushed
the
feat/70-unlock-warnings
branch
from
98a24dc to
421cb6b
Compare
|
I am very confused by this... Why do we need to manually unlock before making requests? There are 2 separate "exception flows" here....One is directed to the developer of the 3rd party app writing the extension integration and one towards the app user/extension owner. First one should throw errors for the dev to handle. Second one should be as frictionless as possible. Cases should be as follows: No wallet created: Any request made should throw with "No wallet found" -> app dev responsibility to notify the user |
apart from whitelisting which is independent of wallet creation |
This PR aims to always request the user log in before attempting to make requests to the extension via content scripts API. If the user is not logged in the expected behavior is that the extension popup will open with account create or password screens as well as warning message in the currently opened tab.
This PR also modifies e2e tests affected by this change as well as attempts to address flaky tests that used timeouts, replacing them with events to avoid different behavior in different computational environments.
The user will be able to make requests successfully if the following conditions are met:
otherwise the popup will request one of the above be corrected.
This PR does not address the issue of queuing requests if the user is logged out - we do not remember previous requests until the user finally succeeds in logging in.
Notes:
Future enhancements: blocking API unauthorized requests could be handled more elegantly via Proxy traps, decorators or interceptors instead of directly in each function. I implemented it this way for simplicity since we require some methods to be functional and others not when the user is logged out, this way we have a simple way of adding blockers in a more granular way.
Addresses issues #70 #44 #59