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.

Sign up for GitHub

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

Jump to bottom

reduce package dependency cycles #6823

Merged
merged 16 commits into from
Jan 24, 2023
Merged

reduce package dependency cycles #6823

merged 16 commits into from
Jan 24, 2023

Conversation

turadg
Copy link
Member

@turadg turadg commented Jan 20, 2023 •
edited
Loading

Description

Progress on #4645

Cycles detected with yarn lerna run build. Graphs drawn with npx lerna-dependency-graph --outputFormat=png

Before
before

12 cycles 
lerna WARN ECYCLE @agoric/swingset-vat -> @agoric/notifier -> @agoric/swingset-vat
lerna WARN ECYCLE agoric -> @agoric/cache -> @agoric/vats -> @agoric/inter-protocol -> @agoric/cosmic-swingset -> agoric
lerna WARN ECYCLE @agoric/deploy-script-support -> (nested cycle: agoric -> @agoric/cache -> @agoric/vats -> @agoric/inter-protocol -> @agoric/cosmic-swingset -> agoric) -> @agoric/deploy-script-support
lerna WARN ECYCLE @agoric/governance -> (nested cycle: @agoric/deploy-script-support -> (nested cycle: agoric -> @agoric/cache -> @agoric/vats -> @agoric/inter-protocol -> @agoric/cosmic-swingset -> agoric) -> @agoric/deploy-script-support) -> @agoric/governance
lerna WARN ECYCLE @agoric/pegasus -> (nested cycle: @agoric/governance -> (nested cycle: @agoric/deploy-script-support -> (nested cycle: agoric -> @agoric/cache -> @agoric/vats -> @agoric/inter-protocol -> @agoric/cosmic-swingset -> agoric) -> @agoric/deploy-script-support) -> @agoric/governance) -> @agoric/pegasus
lerna WARN ECYCLE @agoric/smart-wallet -> (nested cycle: @agoric/pegasus -> (nested cycle: @agoric/governance -> (nested cycle: @agoric/deploy-script-support -> (nested cycle: agoric -> @agoric/cache -> @agoric/vats -> @agoric/inter-protocol -> @agoric/cosmic-swingset -> agoric) -> @agoric/deploy-script-support) -> @agoric/governance) -> @agoric/pegasus) -> @agoric/smart-wallet
lerna WARN ECYCLE @agoric/store -> (nested cycle: @agoric/swingset-vat -> @agoric/notifier -> @agoric/swingset-vat) -> @agoric/store
lerna WARN ECYCLE @agoric/vat-data -> (nested cycle: @agoric/store -> (nested cycle: @agoric/swingset-vat -> @agoric/notifier -> @agoric/swingset-vat) -> @agoric/store) -> @agoric/vat-data
lerna WARN ECYCLE @agoric/zoe -> (nested cycle: @agoric/smart-wallet -> (nested cycle: @agoric/pegasus -> (nested cycle: @agoric/governance -> (nested cycle: @agoric/deploy-script-support -> (nested cycle: agoric -> @agoric/cache -> @agoric/vats -> @agoric/inter-protocol -> @agoric/cosmic-swingset -> agoric) -> @agoric/deploy-script-support) -> @agoric/governance) -> @agoric/pegasus) -> @agoric/smart-wallet) -> @agoric/zoe
lerna WARN ECYCLE @agoric/wallet-backend -> (nested cycle: @agoric/zoe -> (nested cycle: @agoric/smart-wallet -> (nested cycle: @agoric/pegasus -> (nested cycle: @agoric/governance -> (nested cycle: @agoric/deploy-script-support -> (nested cycle: agoric -> @agoric/cache -> @agoric/vats -> @agoric/inter-protocol -> @agoric/cosmic-swingset -> agoric) -> @agoric/deploy-script-support) -> @agoric/governance) -> @agoric/pegasus) -> @agoric/smart-wallet) -> @agoric/zoe) -> @agoric/wallet-backend
lerna WARN ECYCLE (nested cycle: @agoric/vat-data -> (nested cycle: @agoric/store -> (nested cycle: @agoric/swingset-vat -> @agoric/notifier -> @agoric/swingset-vat) -> @agoric/store) -> @agoric/vat-data) -> (nested cycle: @agoric/vat-data -> (nested cycle: @agoric/store -> (nested cycle: @agoric/swingset-vat -> @agoric/notifier -> @agoric/swingset-vat) -> @agoric/store) -> @agoric/vat-data)
lerna WARN ECYCLE (nested cycle: @agoric/wallet-backend -> (nested cycle: @agoric/zoe -> (nested cycle: @agoric/smart-wallet -> (nested cycle: @agoric/pegasus -> (nested cycle: @agoric/governance -> (nested cycle: @agoric/deploy-script-support -> (nested cycle: agoric -> @agoric/cache -> @agoric/vats -> @agoric/inter-protocol -> @agoric/cosmic-swingset -> agoric) -> @agoric/deploy-script-support) -> @agoric/governance) -> @agoric/pegasus) -> @agoric/smart-wallet) -> @agoric/zoe) -> @agoric/wallet-backend) -> (nested cycle: @agoric/wallet-backend -> (nested cycle: @agoric/zoe -> (nested cycle: @agoric/smart-wallet -> (nested cycle: @agoric/pegasus -> (nested cycle: @agoric/governance -> (nested cycle: @agoric/deploy-script-support -> (nested cycle: agoric -> @agoric/cache -> @agoric/vats -> @agoric/inter-protocol -> @agoric/cosmic-swingset -> agoric) -> @agoric/deploy-script-support) -> @agoric/governance) -> @agoric/pegasus) -> @agoric/smart-wallet) -> @agoric/zoe) -> @agoric/wallet-backend)

After

8 cycles 
lerna WARN ECYCLE Dependency cycles detected, you should fix these!
lerna WARN ECYCLE @agoric/swingset-vat -> @agoric/notifier -> @agoric/swingset-vat
lerna WARN ECYCLE @agoric/deploy-script-support -> @agoric/vats -> @agoric/inter-protocol -> @agoric/deploy-script-support
lerna WARN ECYCLE @agoric/governance -> (nested cycle: @agoric/deploy-script-support -> @agoric/vats -> @agoric/inter-protocol -> @agoric/deploy-script-support) -> @agoric/governance
lerna WARN ECYCLE @agoric/smart-wallet -> (nested cycle: @agoric/governance -> (nested cycle: @agoric/deploy-script-support -> @agoric/vats -> @agoric/inter-protocol -> @agoric/deploy-script-support) -> @agoric/governance) -> @agoric/smart-wallet
lerna WARN ECYCLE @agoric/store -> (nested cycle: @agoric/swingset-vat -> @agoric/notifier -> @agoric/swingset-vat) -> @agoric/store
lerna WARN ECYCLE @agoric/vat-data -> (nested cycle: @agoric/store -> (nested cycle: @agoric/swingset-vat -> @agoric/notifier -> @agoric/swingset-vat) -> @agoric/store) -> @agoric/vat-data
lerna WARN ECYCLE (nested cycle: @agoric/smart-wallet -> (nested cycle: @agoric/governance -> (nested cycle: @agoric/deploy-script-support -> @agoric/vats -> @agoric/inter-protocol -> @agoric/deploy-script-support) -> @agoric/governance) -> @agoric/smart-wallet) -> (nested cycle: @agoric/smart-wallet -> (nested cycle: @agoric/governance -> (nested cycle: @agoric/deploy-script-support -> @agoric/vats -> @agoric/inter-protocol -> @agoric/deploy-script-support) -> @agoric/governance) -> @agoric/smart-wallet)
lerna WARN ECYCLE (nested cycle: @agoric/vat-data -> (nested cycle: @agoric/store -> (nested cycle: @agoric/swingset-vat -> @agoric/notifier -> @agoric/swingset-vat) -> @agoric/store) -> @agoric/vat-data) -> (nested cycle: @agoric/vat-data -> (nested cycle: @agoric/store -> (nested cycle: @agoric/swingset-vat -> @agoric/notifier -> @agoric/swingset-vat) -> @agoric/store) -> @agoric/vat-data)

after

Security Considerations

--

Scaling Considerations

--

Documentation Considerations

This breaks some exports, but ones that were unlikely to be used outside the repo.

Testing Considerations

CI

@erights
Copy link
Member

erights commented Jan 21, 2023

batched-deliver.js uses ambient authority (setTimeout, clearTimeout). Even after we isolate separate packages into separate compartments, LavaMoat-like, in order to do least-authority linkage, by moving batched-deliver.js into internal, the internal package as a whole would have to be given this ambient authority. This violates POLA as nothing else in internal needs this authority.

Can batched-deliver.js be fixed so it no longer needs ambient authority? Please?

@turadg turadg force-pushed the ta/reduce-cycles branch 2 times, most recently from 5598a34 to 49d9b11 Compare January 23, 2023 20:36
@turadg turadg added the automerge:no-update (expert!) Automatically merge without updates label Jan 23, 2023
@warner warner mentioned this pull request Jan 23, 2023
Open
michaelfig
michaelfig approved these changes Jan 23, 2023
View reviewed changes
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Looking good.

I would prefer that @agoric/vats/tools/... be renamed to @agoric/internal/tools/... instead of @agoric/internal/src/..., but since we already talked about that, I'm willing to live with it.

golang/cosmos/x/swingset/keeper/keeper.go Outdated
@@ -20,7 +20,7 @@ import (
)

// Top-level paths for chain storage should remain synchronized with
// packages/vats/src/chain-storage-paths.js
// packages/agoric/internal/src/chain-storage-paths.js
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// packages/agoric/internal/src/chain-storage-paths.js
// packages/internal/src/chain-storage-paths.js

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch. I'll force push a fix for this and the other suggestions.

packages/deploy-script-support/test/unitTests/test-assertOfferResult.js
Comment on lines -1 to +2
import { test } from '@agoric/swingset-vat/tools/prepare-test-env-ava.js';
import test from 'ava';
import '@endo/init';
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really the same semantics. Did you do this to be able to drop the devDependency on swingset?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep. the rest of it wasn't totally necessary.

It affects redactions but there's work scheduled to address that broadly endojs/endo#1235

packages/internal/src/lib-chainStorage.js Outdated
@@ -106,7 +142,7 @@ const makeNullStorageNode = () => {
* falling back to an inert object with the correct interface (but incomplete
* behavior) when that is unavailable.
*
* @param {ERef<StorageNode?>} storageNodeRef
* @param {import('@endo/eventual-send').ERef<StorageNode?>} storageNodeRef
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {import('@endo/eventual-send').ERef<StorageNode?>} storageNodeRef
* @param {import('@endo/far').ERef<StorageNode?>} storageNodeRef

@turadg turadg removed the automerge:no-update (expert!) Automatically merge without updates label Jan 23, 2023
turadg added 15 commits January 23, 2023 15:15
@turadg turadg added the automerge:no-update (expert!) Automatically merge without updates label Jan 23, 2023
@mergify mergify bot merged commit 5f1abde into master Jan 24, 2023
@mergify mergify bot deleted the ta/reduce-cycles branch January 24, 2023 02:39
@turadg turadg mentioned this pull request Jan 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelfig michaelfig michaelfig approved these changes

@gibson042 gibson042 Awaiting requested review from gibson042

Assignees
No one assigned
Labels
automerge:no-update (expert!) Automatically merge without updates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@turadg @erights @michaelfig