-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add command marketplace #817
Conversation
1137d4b
to
a30d3cb
Compare
a30d3cb
to
3ba7807
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.
In general really good, just put few feedback in comments and some additional here.
Seems like the go format is broken on the CI, I have few warning in the editor, missing comments, bad indentation... please go format again.
More related to the PR, I feel that the commands are not complete. We can publish informations on the marketplace but don't have the results from it. Knowing if the transaction is sent is one thing but knowing the result is another and knowing the index of the offer, the version of the service etc... is quite important.
Also my feeling is that there is a lot of logic in the CLI that should be moved into the marketplace system service (part of my comment about error handling on the marketplace too). The CLI should not have the responsibility to validate the author or things like that.
Nothing blocking for this PR but definitely some stuff to consider. Otherwise all good :)
return err | ||
} | ||
|
||
if !strings.EqualFold(c.service.Owner, c.account) { |
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.
Fine but I think this check should be done by the system service marketplace, the CLI should just call the createOffer
task without having to do any check on the data
if err != nil { | ||
return err | ||
} | ||
fmt.Printf("%s Offer created with success\n", pretty.SuccessSign) |
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 we have these commands in the CLI we need to display all relevant information. Here for example the index of the offer. Here I cannot from the CLI only do anything after that
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.
Yes you are right. The complicated thing is the marketplace service is returning a generic transaction receipt. It is not properly decoded because it doesn't know which method / event has been called or triggered.
I will work on a solution, but this is not a blocking thing for this PR.
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.
Not blocking for this PR for sure but one solution is to listen at the event from the marketplace.
That's the implementation i did for the js cli. Like that we have the confirmation with the receipt (but we don't care much) and we have the event with the relevant data from the event from the smart contract.
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.
here is an example for this implementation based on event
feature/command-marketplace...feature/command-marketplace-with-listen
…ketplace provider
…. Improve marketplace commands example
I see that CodeClimate throw an gofmt error but when running it locally I don't get any error. Can you tell me what is it or directly fix it? |
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 code seems fine 👍 i'll finish my review after completing my manual tests
eda56c1
to
bf4f214
Compare
diff --git a/commands/provider/marketplace_provider.go b/commands/provider/marketplace_provider.go
index 0cf3bc30..61afba8c 100644
--- a/commands/provider/marketplace_provider.go
+++ b/commands/provider/marketplace_provider.go
@@ -25,9 +25,9 @@ func NewMarketplaceProvider(c coreapi.CoreClient) *MarketplaceProvider {
func (p *MarketplaceProvider) PublishServiceVersion(sid, manifest, manifestProtocol, from string) (Transaction, error) {
input := marketplacePublishServiceVersionTaskInputs{
marketplaceTransactionTaskInputs: marketplaceTransactionTaskInputs{From: from},
- Sid: sid,
- Manifest: manifest,
- ManifestProtocol: manifestProtocol,
+ Sid: sid,
+ Manifest: manifest,
+ ManifestProtocol: manifestProtocol,
}
var output Transaction
return output, p.call("publishServiceVersion", input, &output)
@@ -37,9 +37,9 @@ func (p *MarketplaceProvider) PublishServiceVersion(sid, manifest, manifestProto
func (p *MarketplaceProvider) CreateServiceOffer(sid string, price string, duration string, from string) (Transaction, error) {
input := marketplaceCreateServiceOfferTaskInputs{
marketplaceTransactionTaskInputs: marketplaceTransactionTaskInputs{From: from},
- Sid: sid,
- Price: price,
- Duration: duration,
+ Sid: sid,
+ Price: price,
+ Duration: duration,
}
var output Transaction
return output, p.call("createServiceOffer", input, &output)
@@ -49,8 +49,8 @@ func (p *MarketplaceProvider) CreateServiceOffer(sid string, price string, durat
func (p *MarketplaceProvider) Purchase(sid, offerIndex, from string) ([]Transaction, error) {
input := marketplacePurchaseTaskInputs{
marketplaceTransactionTaskInputs: marketplaceTransactionTaskInputs{From: from},
- Sid: sid,
- OfferIndex: offerIndex,
+ Sid: sid,
+ OfferIndex: offerIndex,
}
var output marketplacePurchaseTaskOutputs
return output.Transactions, p.call("purchase", input, &output) This fixes codeclimate but then triggers and error with the circle ci lint. |
which version of go do you use? |
same here |
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.
Good for me, let's just make sure the current comments are tracked and can be done in different PR.
- Move some verification logic from the CLI to the service
- Listen confirmation event from the service in the CLI to display results
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.
really excited about finally having marketplace commands integrated. 🎊
manual tests are ok.
we definitely need to do some improvements like @antho1404 suggested, for example, moving logic out as much as possible from CLI but they can be done within another PRs. we need some user experience improvements as well, like:
- error messages should be more user friendly.
- we can have the
--yes
flag for approving service publish to skip terminal prompts. - publish command only supports deploying from path but it still is able to receive urls and there is no error for this. when a git url or tarball provided, publish process starts but gets broken after a while (while creating manifest)
Merging now, @NicolasMahe make sure to see all comments |
Dependent on #810
Deployed at address
0x94F4cB92Fe9f547574Aec617B1594B13aBd47Ad3
with token address0x5861B3DC52339d4f976B7fa5d80dB6cd6f477F1B
.Procedure to test:
Import account that has MESG token into wallet
Deploy
Try to deploy a paid service
See error message
Purchase service
Deploy again
Should work 🎊 🍾 (only for 3min because offer is only for 3min)
Publish a service on the marketplace
Publish
Create offer