endo icon indicating copy to clipboard operation
endo copied to clipboard

refactor(common): import checkers from common

Open erights opened this issue 5 months ago • 1 comments

Staged on #2746

Closes: #XXXX Refs: #XXXX

Description

Add a description of the changes that this PR introduces and the files that are the most critical to review.

Security Considerations

Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls?

Scaling Considerations

Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated?

Documentation Considerations

Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users?

Testing Considerations

Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet?

Compatibility Considerations

Does this change break any prior usage patterns? Does this change allow usage patterns to evolve?

Upgrade Considerations

What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed?

Include *BREAKING*: in the commit message with migration instructions for any breaking change.

Update NEWS.md for user-facing changes.

Delete guidance from pull request description before merge (including this!)

erights avatar Jun 10 '25 17:06 erights

Without diving into this PR just yet, I'd like to draw your attention to #2285 (emphasis mine):

I think we'd be better off replacing (cond: boolean, details?: Details | undefined) => boolean "Checker" parameters with optional (template: TemplateStringsArray | string[], ...args: any[]) => false "Rejector" parameters, recovering the old local pattern but without local function creation.

And per #2285 (comment) , we'll also want to rename internal functions for indicating the progress of this transition.

-const checkSomething = (candidate, check = undefined) => {
+const confirmSomething = (candidate, reject = false) => {
   return (
     isOk(candidate) ||
-    (!!check && CX(check)`…`)
+    (reject && reject`…`)
   );
 };

And at the highest possible level, we'd replace use of x => x checkers with undefined rejectors and assertChecker with assert.Fail.

-    checkSomething(candidate, assertChecker)
+    confirmSomething(candidate, Fail)

If any exports do expose a Checker parameter, we'll need to consider a deprecation strategy.

gibson042 avatar Jun 10 '25 17:06 gibson042

To be closed in favor of a redo based on #2911

erights avatar Jul 27 '25 02:07 erights

Closing in favor of https://github.com/endojs/endo/pull/2914

erights avatar Jul 31 '25 02:07 erights