-
Notifications
You must be signed in to change notification settings - Fork 8
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
Noel/libp2p entrypoint #398
Conversation
9fa07fb
to
ecadde4
Compare
How to initialize libp2p daemon with participant keypairsSee https://github.com/libp2p/go-libp2p-daemon for installing
Use the following command to generate sample private keys:
|
2fff30f
to
458f78f
Compare
4f694c3
to
e700e92
Compare
2f9ebe0
to
b9cda36
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.
Some comments in-line. Seems reasonable at a high level.
cli/interaction.ss
Outdated
(dest-address dest-address) | ||
(my-nickname my-nickname) | ||
(contacts contacts))) | ||
(def off-chain-channel (init-off-chain-channel channel-options)) ; FIXME: Teardown |
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 think the fixme could stand to be spelled out a bit more; what exactly needs to happen? (It looks like there's something related in the finally
block).
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.
Was already fixed in the finally
block, forgot to remove the FIXME
.
Have removed it.
runtime/participant-runtime.ss
Outdated
(def (make-libp2p-socket-path address: address host-address: host-address) | ||
(def multiaddr-hash | ||
(let (address-str (.call Address .string<- address)) | ||
(u8vector->base64-string (sha256 (string-append address-str host-address))))) |
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.
Should probably add urlsafe?: #t
here; default base64 can include slashes.
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.
Removed this function (make-libp2p-socket-path
), it has become obsolete.
For future reference, if I were to use u8vector->base64-string
, the urlsafe
option can be found here: https://cons.io/reference/text.html#subu8vector-base64-string
runtime/participant-runtime.ss
Outdated
(u8vector->base64-string (sha256 (string-append address-str host-address))))) | ||
(string-append "/tmp/libp2p-socket-" multiaddr-hash)) | ||
|
||
(def (with-seckey-tempfile nickname: nickname c) |
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.
The variable name c
seems like an odd choice here; maybe something more descriptive?
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.
Updated.
runtime/participant-runtime.ss
Outdated
('libp2p (let () | ||
(def my-nickname (hash-get options 'my-nickname)) | ||
(def host-address (hash-get options 'host-address)) | ||
(def dest-address (hash-get options 'dest-address)) |
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.
In general, use hash-ref
instead of hash-get
unless you actually want it to return #f
if the key is not present (hash-ref
throws an exception). This has been the source of more than one bug in the past.
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.
Updated. Thanks!
runtime/participant-runtime.ss
Outdated
(force-output)) | ||
|
||
;; TODO: Generalize & Upstream to gerbil-utils/json | ||
(def (escape-json-string json-string) |
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.
There are a couple things wrong here:
- Conceptually, this is really supposed to be doing shell escaping; it has nothing to do with json per se.
- More importantly, it does not correctly escape things for the shell. E.g:
"$(rm -rf /) ''"
would be passed through unchanged, since it contains a single quote.- If you got rid of that first case, then
"'$(rm -rf /)'"
would still slip by the escaping in the second case; due to the way shell escaping works, you can unquote back into an interpreted context. So you also need to substitute out any single quotes in the middle of the string.
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.
Renamed this function to be more specific, and added your remarks inline as a TODO, before this can generalized as a way to shell-escape json strings.
runtime/participant-runtime.ss
Outdated
(loop)))))) | ||
|
||
|
||
;; ------------------ Libp2p client methods |
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.
This could do with a link to some relevant documentation; this all seems very magical, not knowing a-priori much about the protocol.
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.
Added more documentation, thanks for pointing this out.
runtime/participant-runtime.ss
Outdated
;; (write-u8 #x1b) | ||
;; (displayln "[0m") | ||
;; (display "> ") | ||
;; (displayln "*** Done") |
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.
Should probably just get rid of commented out code like this (and similarly in chat-handler
below).
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.
Removed
;; Wait for handshake to come in | ||
(thread-sleep! 10) | ||
;; (displayln "2") | ||
;; (force-output) |
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.
Should not leave code commented out like this.
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.
Removed
@@ -0,0 +1,219 @@ | |||
(export #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.
I'm kindof wary of continuing to copy & paste the bulk of the logic for these integration tests; it might be prudent to try to factor some of this out into a function.
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.
Refactored the boilerplate for setting up the test environment into a function. The other areas seem to be specific to the integration test itself (options we pass in etc...)
e517484
to
75702ef
Compare
d0ff975
to
c00abcc
Compare
(if host-addrs ["-hostAddrs" host-addrs] [])... | ||
options ...])) | ||
(cmd (format "{ echo ~a ; exec ~a ; } < /dev/null >> ~a 2>&1" | ||
raw-cmd raw-cmd p2pd-log-path)) |
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.
A more direct way to print the executed commands would be to just pass -x
to the shell.
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.
A couple more minor things, but this mostly looks reasonable to me.
;; TODO: Once the above changes are upstreamed for `start-libp2p-daemon!', | ||
;; this command can be made obsolete too. | ||
(def (open-libp2p-client host-addresses: (host-addresses #f) options: (args []) address: (sock #f) wait: (timeo 12) (path #f)) ;; Extra arguments host-address and options | ||
(let (d (start-libp2p-daemon! host-addresses: host-addresses options: args address: sock wait: timeo)) ;; Should go with host-address/tranpsort/port |
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.
These lines are very long and it makes it hard to read; maybe wrap them, with one key/value per line?
;; See: https://docs.libp2p.io/concepts/protocols/#handler-functions | ||
(def chat-proto "/chat/1.0.0") | ||
|
||
;; This function reads the |
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.
You seem to have lost part of this sentence.
help: "command to use to transfer handshakes") | ||
;; TODO: Abstract into Enum - See gerbil-poo | ||
;; enum off-chain-channel = 'stdio | 'libp2p | ||
(option 'off-chain-channel-selection "-C" "--off-chain-channel" default: 'stdio |
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.
What does the conversion from string to symbol?
;; FIXME: Mark off-chain-channel as experimental, print it in a console prompt | ||
(def channel-options | ||
(hash | ||
(off-chain-channel-selection (symbolify off-chain-channel-selection)) |
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.
maybe validate that?
I was having issues running this initially, however it worked when I did I think something having to do with /ip not being a protocol |
Implement Barebones of: #106.
Run
buy_sig
over libp2pTo run the
buy_sig
interaction over libp2p, in one terminal run:You will be prompted:
Copy that into
dest
in the second terminal, and run:Select the relevant options to run the
buy_sig
interaction.Run the
buy_sig
over libp2p test case locallyProgress
Use user-supplied keypair to derive peerid.
See: Noel/libp2p entrypoint #398 (comment)
gerbil-ethereum/known-addresses.ss
.options: ["-id" filename]
.Backend support and CLI to ensure the go-libp2p-daemon and/or JS library is started with the correct parameters (toggling libp2p, keypairs).
start-interaction
entrypoint to take in an option, which toggles off-chain communication betweenstdout
andlibp2p
.A: No, it is determined before the agreement itself,
Since it specifies the channel through which the agreement is sent.
Backend support and CLI to send and receive handshakes and agreements.
Polishes
Priority
- [] Randomly generate keypair file.