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

Clean up code for handling decryption failures #4126

Merged
merged 11 commits into from
Mar 22, 2024

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Mar 21, 2024

Various improvements, including:

  • Defining an enum for decryption failure reasons
  • Exposing the reason code as a property on Event

Review commit-by-commit.

Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Looks good; one question. Thanks!

@@ -211,12 +214,11 @@ export class DecryptionError extends Error {
public readonly detailedString: string;

public constructor(
public readonly code: string,
Copy link
Member

Choose a reason for hiding this comment

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

Is this a breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well first of all, this class (and indeed the whole file) is supposed to be internal, so anyone using it can keep both halves when it breaks.

In any case: It would only be a breaking change for things constructing a DecryptionError: anything reading code once it is created will implicitly cast the DecryptionFailureCode to a string. And I've not been able to find any code constructing a DecryptionError outside the js-sdk.

@richvdh
Copy link
Member Author

richvdh commented Mar 22, 2024

However, it turns out the removal of MatrixEvent.badEncryptedMessage is a breaking change, despite it being a private method, because there is a JS file in the react SDK which uses it. So I need to do a bit more work here.

@richvdh
Copy link
Member Author

richvdh commented Mar 22, 2024

Once matrix-org/matrix-react-sdk#12364 lands, this should be good to go too.

@richvdh
Copy link
Member Author

richvdh commented Mar 22, 2024

going to override the coverage gate on this

@richvdh richvdh merged commit d1259b2 into develop Mar 22, 2024
22 of 23 checks passed
@richvdh richvdh deleted the rav/historical_events/00_prep branch March 22, 2024 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants