Skip to content
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

Ss/marketplace decode receipt #835

Closed
wants to merge 7 commits into from
Closed

Conversation

NicolasMahe
Copy link
Member

This PR mainly add 3 new tasks to decode the transaction receipt and return useful information about what the transaction did.
It also detailed the return data of task sendSignTransaction and merge the different events into one data struct.

I will create another PR that implement this modification in the marketplace commands.

This PR is the service side of stuff needed for this comments:
https://github.com/mesg-foundation/core/pull/817/files/a56eb0e3aa55f2fc435a0e76537b7c52eb4375c9#r266858115
https://github.com/mesg-foundation/core/pull/817/files/a56eb0e3aa55f2fc435a0e76537b7c52eb4375c9#r266861843

@antho1404
Copy link
Member

I was thinking something based on the event for the command and this, the refactoring for the events is good but I don't see the need of creating the tasks for this. The service should expose the data and task really necessary and should abstract as much as possible to technology it uses.

For me this service should only expose data and tasks related to the data of the marketplace not to ethereum (except maybe the transaction to sign and propagate). So adding tasks here about processing data from eth is not necessary especially that we have these data with the events and we just need to listen the events.

@NicolasMahe
Copy link
Member Author

I was thinking something based on the event for the command and this, the refactoring for the events is good but I don't see the need of creating the tasks for this. The service should expose the data and task really necessary and should abstract as much as possible to technology it uses.

For me this service should only expose data and tasks related to the data of the marketplace not to ethereum (except maybe the transaction to sign and propagate). So adding tasks here about processing data from eth is not necessary especially that we have these data with the events and we just need to listen the events.

There is no way to fully link a transaction to the event, except by decoding the event from the log of the transaction receipt (what this PR is doing).

As there is no know index, it will be a big guess to associate and triggered event from the execution of a task. What happen when many people are using the smart contract at the same time? Then some execution of task will return the result of other. Otherwise, it needs to listen until a event match some criteria, but i feel it's overcomplicated.

@NicolasMahe
Copy link
Member Author

NicolasMahe commented Mar 21, 2019

The best way to get a nice result with only one function, will be to merge, for example, createServiceOrder, with ethwallet.signTransaction, sendSignedTransaction and decodeLogCreateServiceOrder. But to do so, it will require a service composition to use signTransaction of service ethwallet.

In the meantime, I can merge sendSignedTransaction and decodeLogCreateServiceOrder (same for purchase and publishService). But it reduce the flexibility of sendSignedTransaction.

@antho1404
Copy link
Member

FYI, here is a proof of concept that solves the problem with the CLI and doesn't need any modification here feature/command-marketplace...feature/command-marketplace-with-listen

# Conflicts:
#	systemservices/marketplace/src/contracts/utils.ts
@NicolasMahe
Copy link
Member Author

closing in favor of #844

@NicolasMahe NicolasMahe deleted the ss/marketplace-decode-receipt branch March 29, 2019 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants