-
Notifications
You must be signed in to change notification settings - Fork 208
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
chore(network): extract network implementation from SwingSet #8151
Conversation
9dae20d
to
0da93e4
Compare
d29ac6a
to
681c359
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved contingent on
- maintaining history of the files in
src/vats/network
- fixing the 'uuuu' comment
packages/pegasus/src/pegasus.js
Outdated
@@ -314,7 +313,7 @@ const makePegasus = (zcf, board, namesByAddress) => { | |||
|
|||
return Far('pegasus', { | |||
/** | |||
* Return a handler that can be used with the Network API. | |||
* Return a handler that can be used with the uuuuuuuu API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intermediate refactor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heheh... leaned on the keyboard at the wrong time. :)
scripts/graph.sh
Outdated
else | ||
echo "Cycles detected. These lines appear only in the original graph and not the acyclic variant:" | ||
echo 1>&2 "Cycles detected. These lines appear only in the original graph and not the acyclic variant:" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kinda weird to have stderr say "these:" referring to stdout because a user could end up seeing one stream and not the other. Consider reverting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the commit message rationale now, that stdout should have parseable output. That's persuasive to me.. If this script was going to live longer I'd say to update the message to not assume both streams will be seen at once, but I don't really think it's worth bothering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a cheap fix, and I have to make some other little doc changes, so I'll update the message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please try to get Git to see that multiaddr.js
and types.js
moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you browse the PR commit-by-commit, you see that Git believes they moved in the first commit, but as soon as you collapse all the commits via a diff, Git's "50% similarity means it moved" heuristic breaks because these are small files with a bunch of lint fixes.
"ava": "^5.3.0", | ||
"c8": "^7.13.0" | ||
}, | ||
"exports": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super.
i forget, does our tooling still allow deep imports anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found Typescript does, but Endo and Node.js definitely don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very nice.
Turadg already caught the two things I noticed (uuuuuu and file movement).
packages/network/README.md
Outdated
# Network API | ||
<!-- | ||
content should remain synchronized with | ||
https://github.com/Agoric/documentation/blob/HEAD/main/repl/networking.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
404 Not Found, did you intend a different URL?
https://github.com/Agoric/documentation/blob/HEAD/main/repl/networking.md | |
https://github.com/Agoric/documentation/blob/main/main/reference/repl/networking.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was already broken when I got here, but thanks for noticing; updated!
681c359
to
242f953
Compare
242f953
to
6b063a8
Compare
chore(network): extract network implementation from SwingSet
refs: #5973
Description
Move the
packages/SwingSet/src/vats/network
implementation topackages/network/src
. There was no good reason to put it in SwingSet initially.This is a step toward durability for the network and IBC vats (
vat-network.js
,vat-ibc.js
) without introducing cycles from SwingSet to the Zones.Security Considerations
n/a
Scaling Considerations
n/a
Documentation Considerations
Moves the location of the network documentation from
SwingSet/docs/networking.md
tonetwork/README.md
, but leaves a forwarding link.Testing Considerations
n/a
Upgrade Considerations
n/a