Execution SDK on Cosmos #1463
Merged
Execution SDK on Cosmos #1463
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NicolasMahe
reviewed
Nov 5, 2019
4 tasks
NicolasMahe
force-pushed
the
feature/execution-store
branch
from
62fec93
to
6fb82d4
Compare
…its the appropriate cosmos events and manage errors.
NicolasMahe
force-pushed
the
feature/execution-store
branch
from
6a2bd92
to
3216db2
Compare
- remove/comment panics - handle channel closing - move filter related methods to api package - change app.handler return type - add valid method for hash - move some part to x packages
krhubert
force-pushed
the
feature/execution-store
branch
from
1b0bf06
to
1da0b08
Compare
NicolasMahe
reviewed
Nov 20, 2019
NicolasMahe
reviewed
Nov 20, 2019
NicolasMahe
reviewed
Nov 20, 2019
NicolasMahe
reviewed
Nov 20, 2019
NicolasMahe
reviewed
Nov 20, 2019
NicolasMahe
reviewed
Nov 20, 2019
NicolasMahe
reviewed
Nov 20, 2019
krhubert
force-pushed
the
feature/execution-store
branch
from
0157e3b
to
e78699b
Compare
NicolasMahe
reviewed
Nov 21, 2019
| } else { | ||
| hashC <- hash | ||
| } | ||
| case <-ctx.Done(): |
NicolasMahe
reviewed
Nov 21, 2019
|
Update of the js libs: mesg-foundation/js-sdk#135 |
NicolasMahe
approved these changes
Nov 21, 2019
antho1404
reviewed
Nov 22, 2019
Comment on lines
+145
to
+169
| go func() { | ||
| loop: | ||
| for { | ||
| select { | ||
| case event := <-eventStream: | ||
| tags := event.Events[EventHashType] | ||
| if len(tags) != 1 { | ||
| errC <- fmt.Errorf("event %s has %d tag(s), but only 1 is expected", EventHashType, len(tags)) | ||
| break | ||
| } | ||
|
|
||
| hash, err := hash.Decode(tags[0]) | ||
| if err != nil { | ||
| errC <- err | ||
| } else { | ||
| hashC <- hash | ||
| } | ||
| case <-ctx.Done(): | ||
| break loop | ||
| } | ||
| } | ||
| close(errC) | ||
| close(hashC) | ||
| c.Unsubscribe(context.Background(), subscriber, query) | ||
| }() |
There was a problem hiding this comment.
For code clarity, this could be move as a separated function and like that, we could have the following:
defer func() {
close(errC)
close(hashC)
c.Unsubscribe(context.Background(), subscriber, query)
}()
for {
select {
case <- Done:
return
}
}There was a problem hiding this comment.
@krhubert i let you make the decision ;)
| @@ -123,6 +115,8 @@ func loadOrGenDevGenesis(app *cosmos.App, kb *cosmos.Keybase, cfg *config.Config | |||
| } | |||
|
|
|||
| func main() { | |||
| xrand.SeedInit() | |||
There was a problem hiding this comment.
Not clear what's the reason for this. Is it for the subscriber name that we generate?
There was a problem hiding this comment.
yes. but i think the goal of the package xrand is to centralized on random related function.
So we don't have the send the random everywhere in the code. it's done once and that's it.
| execC := make(chan *execution.Execution) | ||
| errC := make(chan error) | ||
| go func() { | ||
| loop: |
There was a problem hiding this comment.
same comment as before, could be done with a defer and return in the ctx.Done instead of a loop:
There was a problem hiding this comment.
@krhubert i let you make the decision ;)
… yet (issue with new node sync)
antho1404
approved these changes
Nov 22, 2019
NicolasMahe
added
release:change
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves the Execution SDK on Cosmos 🎊
Dependent on #1509 and #1512
You can check the commits after 499b03b:
https://github.com/mesg-foundation/engine/pull/1463/files/499b03b84cf150f0dfe6a72947da81446a6a2f27..02e26b848ab0388a8957b359c1bcd8c24b11f88c