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

Key verification request fixes #954

Merged
merged 1 commit into from
Jun 14, 2019

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Jun 13, 2019

  • fix requestVerification in MatrixClient to match the function in crypto
    • reorder the arguments so that the arguments actually do what they say they
      do
    • pass through the third argument, which was accidentally omitted
  • ignore verification requests from ourselves
  • also fix a comment

- fix requestVerification in MatrixClient to match the function in crypto
  - reorder the arguments so that the arguments actually do what they say they
    do
  - pass through the third argument, which was accidentally omitted
- ignore verification requests from ourselves
- also fix a comment
@uhoreg uhoreg requested a review from a team June 13, 2019 18:31
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is anything using this that's going to be broken by the argument re-ordering?

@uhoreg
Copy link
Member Author

uhoreg commented Jun 14, 2019

Is anything using this that's going to be broken by the argument re-ordering?

No, because even though the arguments are reordered in the MatrixClient method, the order in which they're passed isn't (i.e. the second argument of MatrixClient#requestVerification gets passed as the second argument to crypto#requestVerification), and the order of the crypto versions doesn't change in this PR. So in fact, someone who tried to use the currently documented order of arguments would have wound up with something that doesn't work properly, since it would have passed devices into methods. And given that nobody has complained yet, I can be pretty certain that nobody is using this function with the second or third arguments. ;)

@uhoreg uhoreg merged commit d694ee3 into matrix-org:develop Jun 14, 2019
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