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: modernize @endo/errors @agoric/store imports #8781

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

erights
Copy link
Member

@erights erights commented Jan 21, 2024

closes: #XXXX
refs: #8771

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:

  • Directly importing many things from endo, rather than obsolete agoric-sdk re-exporters
  • Switching many uses of assert from the global or @agoric/assert package to its successor @endo/errors.

There's one trivial semantic change in switching many asserts to @endo/errors. Unlike the global assert or the assert exported by @agoric/assert, the assert object as exported by @endo/errors does not have non-asserting methods like note (aka errorNote) or error (aka makeError). Thus, when these were called as methods on assert, I had to import the functions directly, and then change the code to call the function, rather than making a method call. Other than that, this PR should have very little aside from changing imports and a few reexports + the package.json changes needed to depend on those endo packages.

Extracted from #8771 per #8771 (comment) but made into a separate PR so it can be merged or not on its own.

Also includes @turadg 's fix to the casting maxNodeModuleJsDepth from #8771

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

none

Upgrade Considerations

@ivanlei , I added you for feedback on when it would not be too disruptive to merge this. That's another reason to separate it into a PR distinct from the remaining changes in #8771 . By minimizing the non-import-export code changes, it shouldn't much hurt our ability to cherry pick.

@@ -1,13 +1,12 @@
import { quote as q, throwRedacted as Fail } from '@endo/errors';
Copy link
Member

Choose a reason for hiding this comment

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

This pattern is what I was trying to get away from with making a package at all. There's no export of Fail so there's no autocomplete when someone wants to import Fail.

q and Fail are the convention in this repo, so they should have exports. I propose @agoric/errors assert continue to export those.

The other changes in the PR look good. I'd have accepted without the errors changed. That's one of the good reasons to separate commits. E.g. refactor: modernize @agoric/store imports and separately refactor: modernize errors imports. Then we could easily pick which changes to include.

Incidentally, how did 6e0290e turn into a49db10 ? Why not simply cherry-pick it?

Copy link
Member

Choose a reason for hiding this comment

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

Incidentally, the plan of record is for @agoric/assert to itself import @endo/errors,

@erights erights force-pushed the markm-remove-migrated-common-2 branch from a49db10 to 4a722f8 Compare January 26, 2024 01:58
@erights erights marked this pull request as draft January 26, 2024 06:07
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.

2 participants