-
Notifications
You must be signed in to change notification settings - Fork 172
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
#211 Namespace validation #215
Conversation
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.
nice namespaces validation and tests
take a look at my comments, we can discuss if you don't agree on something
|
||
do { | ||
agreementKey = try kms.performKeyAgreement(selfPublicKey: selfPublicKey, peerPublicKey: proposal.proposer.publicKey) | ||
try Namespace.validate(proposedNamespaces) |
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.
shouldn't proposedNamespaces
be already validated when received proposal?
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.
Actually no, but it should. I'm changing it
try Namespace.validate(sessionNamespaces) | ||
try Namespace.validateApproved(sessionNamespaces, against: proposedNamespaces) |
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 think those two validations should not be in do{}catch{} block but rather throw an error so user can see that minimal requirement(or other requirement) has not been met.
I am also not sure if it make sense to respond with error for them in catch{} block
that is not valid error for a peer.
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 agree with the validation error catch, it has to be thrown. I just used it as a temporary solution to make the validation start working. An issue can be created for that to improve error reading.
The catch respond error already existed before, but only when the key agreement failed. Should this be changed also?
@@ -209,6 +212,7 @@ final class PairingEngine { | |||
try? pairing.updateExpiry() | |||
} | |||
|
|||
onApprovalResponse?(proposal) // FIXME: This proposal needs to be stored on creation and retrieved from cache here. Don't use proposal from response. |
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.
This proposal needs to be stored on creation and retrieved from cache here. Don't use proposal from response.
actually that proposal is from cache(jsonRpcHistory record) not from response, response do not contain proposal.
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.
True, removing this todo.
pairingEngine.onProposeResponse = { [unowned self] sessionTopic in | ||
sessionEngine.setSubscription(topic: sessionTopic) | ||
} | ||
pairingEngine.onApprovalResponse = { [unowned self] proposal in | ||
sessionEngine.settlingProposal = proposal | ||
} |
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.
do you need that extra callback onApprovalResponse
? onProposeResponse
is executed in the same function.
would it work to have two arguments(if needed) in on callback?
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.
Yes, changed the callback to 2 args
@@ -12,6 +12,8 @@ final class SessionEngine { | |||
var onSessionDelete: ((String, SessionType.Reason)->())? | |||
var onEventReceived: ((String, Session.Event, Blockchain?)->())? | |||
|
|||
var settlingProposal: SessionProposal? |
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 it be overriden?
could some runtime key value storage be safer?
private func onSessionSettle(payload: WCRequestSubscriptionPayload, settleParams: SessionType.SettleParams) { | ||
logger.debug("Did receive session settle request") | ||
guard let proposedNamespaces = settlingProposal?.requiredNamespaces else { |
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.
- you set
settlingProposal
onApprovalResponse
so it happens onresponder's
engine as responder calls "approve" - you retrieve that
settlingProposal
onSessionSettle
so it happens onproposer's
engine as responder calls "settle"
🤔 am I correct? will it work?
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.
splitting engines/methods for controller/non-controller later will really help.
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.
Actually they both happen on proposer's side, so it will work. The thing is that they happen on different engines. There may be a better solution to this.
Adds validation functions to new namespaces, applies planned test cases.
Integrates validation on proposal-approval flow.