Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Sign Engines's session request logic #269

Merged
merged 7 commits into from
Feb 23, 2024

Conversation

quetool
Copy link
Collaborator

@quetool quetool commented Feb 20, 2024

Description

There was a bad decision-making process inside Sign Engine in regards of session requests where a wallet could use methods handlers AND onSessionRequest events but not separately.

The data carried by both logics didn't change, method handler will still make use of topic and parameters and onSessionRequest event will still make use of SessionRequestEvent object which contains id, topic, method, chainId and params but by fixing the decision-making logic inside Sign Engine we now resolve this issue #258

APPROVE SESSIONS:

BEFORE PR:

  • Until now, most developers were relying on generatedNamespaces object inside of SessionProposalEvent's params, which is a handy object that determines automatically which methods are supported based on which handlers are registered with registerRequestHandler()

AFTER PR:

  • Now, generatedNamespaces is still a valid object to be used to approve a session but if the wallet developer wants to handle session requests using onSessionRequest events (without registering a handler) then they will need to pass to approveSession() a custom set of namespaces that would include those methods without a registered handler. (This is similar to any other SDK)

HANDLING SESSION REQUESTS:

BEFORE PR:

  • Until now, only by registering request handlers was possible to handle a session (method) request and only after handling it with the registered handler, only then, an onSessionRequest event was emitted (which was useless since the request was already being responded at that point).
  • Furthermore, and more importantly, there was no way of handling the request with onSessionRequest since the onSessionRequest's event was being emitted only if a request handler was registered before and if a request handler was not registered for a given method then that method was considered unsupported. This is wrong!

AFTER PR:

  • Now, if a method handler is registered for a given method then that handler is going to be used to handle the request WITHOUT emitting an onSessionRequest event (which was anyway useless before, as mentioned above)
  • Now, if no method handler is registered for a given method, an onSessionRequest event will be emitted for that request and the request would have to be handled in the onSessionRequest subscription method. If there are no methods handlers registered then every request will emit an onSessionRequest event to be handled.

How Has This Been Tested?

  • Smoke and manual tests.
  • Fixed UT accordingly.

Due Dilligence

  • Breaking change
  • Requires a documentation update

@quetool quetool changed the title fix _onSessionRequest logic in sign_engine so wallet can use either methods handlers or onSessionRequest event Fix Sign Engines's session request logic Feb 20, 2024
@quetool quetool marked this pull request as ready for review February 22, 2024 12:04
@quetool quetool merged commit 40a593a into master Feb 23, 2024
1 check passed
@quetool quetool deleted the bugfix/proper_usage_on_session_request branch April 2, 2024 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants