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

types(marshal): fix #1257 typing of deeplyFulfilled #2312

Merged
merged 4 commits into from
Jun 26, 2024

Conversation

erights
Copy link
Contributor

@erights erights commented Jun 9, 2024

Closes: #1257
Refs: Agoric/agoric-sdk#5993 Agoric/agoric-sdk#6816 #1455

Description

Low urgency. This is a minor reduction of tech debt.

Agoric/agoric-sdk#5993 introduced deeplyFulfilledObject to work around the deficiencies in the typing of deeplyFulfilled explained at #1257 . This PR fixes those deficiencies. This does not necessary make deeplyFulfilledObject unneeded, but it should enable many callers of deeplyFulfilledObject to call deeplyFulfilled instead without loss of type fidelity.

Note: the code in this first commit probably does not use TS typing well, resulting in many internal @ts-expect-error directives in the implementation. This is because I still don't really understand what I'm doing with complex TS types. Reviewers, help appreciated.

But IIUC, the external type should now be as good as deeplyFulfilledObject. Hopefully, the only typing flaws are encapsulated by this implementation. Yes?

Security Considerations

More accurate typing helps security. More precise but inaccurate unsound typing hurts security. Our security practices are already adapted to this dilemma, and this PR should not have much effect.

Scaling Considerations

none

Documentation Considerations

It would be nice if this PR eventually let us drop the need to explain deeplyFulfilledObject and why it co-exists with deeplyFulfilled

Testing Considerations

  • since this is all about changing static types, I need to write some static type tests.

Even though the code itself is also a bit different, these differences are with high confidence a pure refactor, so the existing dynamic tests should adequately test that.

Compatibility Considerations

Assuming that these changes are a pure refactor with no dynamic behavior changes, there should be no dynamic compat issues.

The tighter typing of deeplyFulfilled does raise the possibility that some call sites will no longer pass static type checks. But no such problem appears within the PR's CI, and therefore within the endo repo.

Upgrade Considerations

none.

  • Include *BREAKING*: in the commit message with migration instructions for any breaking change.
  • Update NEWS.md for user-facing changes.

@erights erights self-assigned this Jun 9, 2024
@erights erights changed the title types(marshal): fix #1257 typing of deeplyFulfilled types(marshal): fix #1257 typing of deeplyFulfilled Jun 9, 2024
@erights erights marked this pull request as ready for review June 9, 2024 19:52
@erights erights requested a review from turadg June 9, 2024 19:54
packages/marshal/src/deeplyFulfilled.js Outdated Show resolved Hide resolved
packages/marshal/src/deeplyFulfilled.js Outdated Show resolved Hide resolved
@erights erights force-pushed the markm-1257-type-deeplyFulfilled branch from 06f4f3e to 8da8e11 Compare June 26, 2024 18:28
@erights erights enabled auto-merge (squash) June 26, 2024 18:28
@erights erights merged commit 7a29550 into master Jun 26, 2024
17 checks passed
@erights erights deleted the markm-1257-type-deeplyFulfilled branch June 26, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deeplyFulfilled erases type information
2 participants