-
Notifications
You must be signed in to change notification settings - Fork 180
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
[Sign] Dapp extend session exp #1187
Conversation
@flypaper0 extracting the update/extend method to the one service is a bit tricky, cuz NonController also observes the changes made to the session from the wallet side. I'll think about how to refactor this later, cuz it's P1. |
if session.selfIsController { | ||
try await controllerSessionStateMachine.extend(topic: topic, by: ttl) | ||
} else { | ||
try await nonControllerSessionStateMachine.extend(topic: topic, by: ttl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't be easier to just extract generic "sessionExtendRequester" and have an extend logic in one place?
are the implementations for a dapp and a wallet any different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just extract the extend service as it seems like the logic is the same.
@llbartekll please check again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also maybe now extract the on extend part?
now the logic is shared and we still keep request handling and responding in controller and non-controller services.
in let's say SessionExtendRequestSubscriber - that will handle the request and respond
and if needed SessionExtendResponseSubscriber that will handle the response.
Description
Resolves # (issue)
How Has This Been Tested?
Due Dilligence