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

Chore(suite): refactor FW check selectors #15626

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

Lemonexe
Copy link
Contributor

@Lemonexe Lemonexe commented Nov 28, 2024

Description

Refactor selectors for FW authenticity checks, as they became complex, duplicated and with misleading names.
Most notably, soft errors were called skipped errors, even though they display banner, so they're not rly skipped 🙈

Incidentally, fix small issue from #15555 where sentry reports even skipped errors like check-unsupported. The main purpose of 15555 was to report it even when turned off in settings. But thinking about it again, IMO we should report soft reivision check errors too (also didn't before 15555), but exclude the skipped hash check errors.

  • bac0120 rename skippedErrors to softErrors, because they were used as such, and decouple skippedErrors
  • bbab835 actually use skipped hash check errors in selector to centralize it
  • b9357d7 rename error selectors to errorIfEnabled, and split error from them so they can be used for Sentry
  • 4394bf5 on the other hand, squash selectors used only locally, IMO they don't add much clarity
  • b429b12 introduce another category - errors skipped but still reported to Sentry 🙈
  • 266e7f5 add other-error to that category
  • 2eea204 cleanup: reorganize scenarios for each errors

You can test other-error by checking out testing branch

Notes

I attempted to standardize variable names as follows:

  • we start with the basic definition of any error (of those types). Some of those are:
  • skipped error: is not handled at all
    • no modal, not blocking recieve addr, no banner, no sentry
  • skipped but reported error:
  • soft error: is handled in a non-blocking way
    • no modal, not blocking recieve addr, but will do banner, sentry
  • hard error: fully handled, gets the full treatment
    • modal, blocking receive addr, banner, sentry
    • equals "not soft error"

Still, it's not consistent between revision & hash check because they got different requirements:

  • revision check only has soft and hard, but no skipped
  • hash check has skipped, skipped+reported and hard, but no soft

@Lemonexe Lemonexe force-pushed the chore/refactor-fw-hash-check-selectors branch from 0a62a53 to 6e172eb Compare November 28, 2024 13:51
@Lemonexe Lemonexe marked this pull request as ready for review November 28, 2024 14:29
@Lemonexe Lemonexe marked this pull request as draft November 28, 2024 15:34
@Lemonexe Lemonexe force-pushed the chore/refactor-fw-hash-check-selectors branch 2 times, most recently from f096e71 to 266e7f5 Compare November 29, 2024 09:29
export const skippedHashCheckErrors = [
'check-skipped',
'check-unsupported',
// this could be serious, but it's also caught by revision check, which handles edge-cases better, so it's skipped here
'unknown-release',
] satisfies FirmwareHashCheckError[];

// These errors will be treated softly, with only a warning displayed instead of modal
export const softHashCheckErrors = [...skippedHashCheckErrors];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

softHashCheckErrors will be removed in next commits (no need to support that)

) {
return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

useReportRevisionCheck is now much nicer thanks to the new selector that gets error regardless of enabled.
But useReportHashCheck does not use it, as it also needs payload, so I have to do the whole decision process again.
💭 I could create selectHashCheckErrorPayload but sounds like overkill, what do you think?

reportCheckFail('hash', { ...commonData, hashCheckError, hashCheckErrorPayload });
}, [commonData, hashCheckError, isHashCheckError, hashCheckErrorPayload]);
}, [commonData, hashCheckError, hashCheckErrorPayload]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

📝 note: I tried passing just hashCheck to the useEffect, and declaring hashCheckError, hashCheckErrorPayload within the useEffect body.
At first glance it's nicer, as I can early return & avoid the ternaries.
🚫 But hashCheckobj from redux is referentially unstable, so it Sentry fires repeatedly

@Lemonexe Lemonexe marked this pull request as ready for review November 29, 2024 09:44
@Lemonexe Lemonexe force-pushed the chore/refactor-fw-hash-check-selectors branch from 266e7f5 to 2eea204 Compare November 29, 2024 17:08
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.

1 participant