-
Notifications
You must be signed in to change notification settings - Fork 288
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
feat(cactus-api-client): common verifier-factory #1879
Conversation
@petermetz I'm still working on this task, but the core design proposal is complete, so please review in spare time and let me know what to clarify or improve :) The interesting parts are:
I will add more tests and do some more improvements tomorrow. |
f94ea46
to
56a8f52
Compare
There's another branch where I've adjusted electricity-trade to use new verifier factory (sort of sanity test). It breaks dockerization though, so it's not included here (need a simpler way to use local cactus packages, without pushing them to npm first). To start the sample just follow the readme as usual, open another shell, Branch - https://github.com/outSH/cactus/tree/electricity_trade_common_verifier |
56a8f52
to
8c450ac
Compare
2b57f69
to
534099f
Compare
@outSH OK, cool. The following is a not a change request, just something to ponder: Making a package that is intended to re-export literally every connector package (of which the number is to grow infinitely by design) will have a bundle size trending to infinity as well. This is especially important for browser based applications where the end user experience depends heavily on bundle size. |
@petermetz Yes, that was my concern few messages above as well, but it's a requirement from one of the teams already using verifier interface. In the follow up PR I want to investigate possibility of importing apiclient modules dynamically, but this is not critical now when we only import 2 or 3 clients. |
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
@outSH I guess in an ideal situation they would understand that using pre-1.0.0 software means that there will be breaking changes, but I know that's not how you roll over there so that's totally fair. |
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.
@outSH LGTM, thank you for the updates and putting up with my nit-picking as usual!
Adjust SocketIOApiClient and common Verifier behavior to previous one in cmd-socketio, fix related tests. Extract Verifier interfaces to common location to ensure interface compatibility between old and new verifier / verifier factory (both should implement same interface). Create new package cactus-verifier-client for common verifier related stuff, to prevent circular dependencies. Add verifier-factory to create verifiers by supplying it's ID only, based on initial configuration. Configuration is set in ctor, but can be read from a file as in cmd-socketio scenarious. VerifierFactory config should be compatible with existing ledgerPluginInfo. Closes: hyperledger-cacti#1878 Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
af0b9de
to
fff07d0
Compare
Signed-off-by: Michal Bajer michal.bajer@fujitsu.com