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

refactor: postponed removals and simplifications #8771

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

erights
Copy link
Member

@erights erights commented Jan 18, 2024

Staged on #8781

closes: #XXXX
refs: #8749 endojs/endo#1985 #8781

Description

Now that a recent endo is released and agoric-sdk has been upgraded to depend on it, we can finally get to a lot of agoric-sdk technical-debt cleanups that we've postponed. The cleanups in this PR include:

  • Deleting redundant local copies of both types and code; instead importing them from the appropriate endo package.
  • Deleting the entire @agoric/same-structure package, that had no remaining users anyway.

Security Considerations

Less code helps security. No other effect on security

Scaling Considerations

none. (or minor help due to smaller total code)

Documentation Considerations

Once these reductions are complete and we can delete legacy exporters, there will be less to document.

Testing Considerations

Of the redundant code copies here, the tests here a bit bigger than the ones in their new home. So endojs/endo#1985 will preserve this extra test code.

Upgrade Considerations

@ivanlei added you as a reviewer for advice on when it would not be too disruptive to merge this.

@erights erights self-assigned this Jan 18, 2024
@erights erights force-pushed the markm-remove-migrated-common branch 8 times, most recently from 3ec1016 to 1b5fbf7 Compare January 20, 2024 03:13
@erights erights marked this pull request as ready for review January 20, 2024 03:48
@erights
Copy link
Member Author

erights commented Jan 20, 2024

@turadg , at https://github.com/Agoric/agoric-sdk/actions/runs/7591791789/job/20680380230?pr=8771#step:4:361 I'm getting one block of type errors I'd like your help on. These errors only happen indirectly in the @agoric/casting package. There are no such errors in the packages these things are imported from. Nor in any other packages that import them. What's special about @agoric/casting?

@erights
Copy link
Member Author

erights commented Jan 20, 2024

At endojs/endo#1933 (comment) , I ask about possible interaction with some endo side changes that were also waiting until after the endo-agoric-sdk sync.

@erights erights requested a review from kriskowal January 20, 2024 04:39
@erights erights changed the title refactor: remove migrated common refactor: postponed removals and simplifications Jan 20, 2024
@turadg
Copy link
Member

turadg commented Jan 20, 2024

What's special about @agoric/casting?

Nothing particularly special. Each package is a separate TypeScript project so they each resolve differently (see #4404 as a spike to unify).

In this case @agoric/casting has a package search depth of 2 (maxNodeModuleJsDepth) and that leads to import some types that themselves have dependencies, which cannot be found because they require searching deeper.

We rely on searching because of the decision years ago to rely on ambient types. The more we use type modules (with imports and exports) the less we'll have this problem.

One reason it's happening in your PR is that making @agoric/assert depend on @endo/errors increased the search depth necessary to resolve the the assert type. So moving more to @endo/errors helps. I've pushed 6e0290e including that.

The change also reduces the maxNodeModuleJsDepth to 1, shrinking the type import graph. That caused another problem, with ThisType in @agoric/notifier but I punted that with ts-ignore and XXX comment justifying it.

Separately, your "cleanups in this PR" list has several items but so far it's all in one commit. Please consider separating the changes by commit. It will be easier to review that way.

@erights erights force-pushed the markm-remove-migrated-common branch from 6e0290e to ac7afd2 Compare January 21, 2024 02:10
@erights erights changed the base branch from master to markm-remove-migrated-common-2 January 21, 2024 02:10
@erights erights requested a review from ivanlei January 21, 2024 02:24
@erights
Copy link
Member Author

erights commented Jan 21, 2024

Separately, your "cleanups in this PR" list has several items but so far it's all in one commit. Please consider separating the changes by commit. It will be easier to review that way.

The vast majority of the bulk were changing imports, and use of the assert global, to instead import from appropriate endo packages. All that has not been moved to #8781 . Done.

Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

This looks fine to me.

My impression of the current intended process is that we can merge changes to master as they get approved, and the hard version compatibility questions will be answered as we decide when to migrate to a release branch. @warner, is that a correct reading of the theory?

@erights erights force-pushed the markm-remove-migrated-common-2 branch from a49db10 to 4a722f8 Compare January 26, 2024 01:58
@erights erights force-pushed the markm-remove-migrated-common branch from ac7afd2 to 1c8af6d Compare January 26, 2024 01:59
@erights erights changed the base branch from markm-remove-migrated-common-2 to master January 26, 2024 03:48
@erights erights force-pushed the markm-remove-migrated-common branch 3 times, most recently from f4b29d2 to b5120d8 Compare January 26, 2024 04:09
@erights erights force-pushed the markm-remove-migrated-common branch from b5120d8 to d59dc41 Compare January 26, 2024 04:54
@erights erights added automerge:squash Automatically squash merge force:integration Force integration tests to run on PR automerge:rebase Automatically rebase updates, then merge and removed automerge:squash Automatically squash merge labels Jan 26, 2024
@erights
Copy link
Member Author

erights commented Jan 26, 2024

@Mergifyio requeue

Copy link
Contributor

mergify bot commented Jan 26, 2024

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@mergify mergify bot merged commit eddf36d into master Jan 26, 2024
92 of 100 checks passed
@mergify mergify bot deleted the markm-remove-migrated-common branch January 26, 2024 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants