-
Notifications
You must be signed in to change notification settings - Fork 10
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
publish results over legs #359
Conversation
* [ ] waiting for [go-ds-sql](ipfs/go-ds-sql#25) datastore compatibility for use as the datastore behind the legs storage
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.
LGTM, but I personally would make the suggested change around setting up legs
@@ -144,6 +148,19 @@ func newStateDBWithNotify(ctx context.Context, dbConn DBConnector, migrator Migr | |||
log.Infow("recovered scheduled tasks", "task_count", count) | |||
} | |||
|
|||
storeLS := storeLS(st.Store(context.Background())) |
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.
part of me would suggest exposing the datastore here, but having instantiation of libp2p and legs be elsewhere. seems like an odd mixing of concerns to setup a whole network transport stack inside of a database instantiation.
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 could be wrong but I don't think I actually see any real common deps other than the priv key?
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 good point. i just don't know where i actually want to make it the legs/libp2p host :)
} | ||
|
||
b := dbDS("legs_data", st.db()) | ||
pub, err := dtsync.NewPublisher(host, b, storeLS, "/dealbot/v1.0.0") |
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.
Can we make the topic configurable?
Side note: should this be Pando topic or is the expectation that Pando producers can also specify topic?
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.
happy to use a pando topic if there is one
Co-authored-by: Masih H. Derkani <m@derkani.org>
Relates to #342