Conversation
ebarakos
left a comment
ebarakos
left a comment
There was a problem hiding this comment.
Seems good, apart from the fact that there exists some duplication in WS files. Can we do something to avoid this?
| }, | ||
| { | ||
| "skips unfiltered WS Oracle request", | ||
| fields{jobid: "Z9999"}, |
There was a problem hiding this comment.
What is this Z here? Moved by the older tests, just wondering if it is intentional or something.
There was a problem hiding this comment.
I think it's intentional since it tests this line in the runlog file:
if query.JobID != r.jobid {
return nil, errNoJobMatch
}| Addresses: addresses, | ||
| } | ||
| case agoric.Name: | ||
| // TODO: No data stored here? |
There was a problem hiding this comment.
So, we don't need any data for this subscription?
There was a problem hiding this comment.
I guess not? In the original implementation the stored data is also empty...
| Client | ||
| // CoreWS are pure WS connections without | ||
| // any JSON-RPC support | ||
| CoreWS |
There was a problem hiding this comment.
What is the difference between this and WS above? Could we merge them?
| @@ -0,0 +1,112 @@ | |||
| package subscriber | |||
There was a problem hiding this comment.
Maybe core_ws.go could be utilized inside ws.go so we avoid duplication? I see quite similar parts.
There was a problem hiding this comment.
I like the idea of utilizing core_ws inside ws seems more efficient instead of duplicating functionality. Currently taking a look
| @@ -1,221 +0,0 @@ | |||
| package blockchain | |||
There was a problem hiding this comment.
What about some of the other tests here?
| if err != nil { | ||
| logger.Error(err) | ||
| var fac time.Duration | ||
| if attempts < 5 { |
There was a problem hiding this comment.
Maybe these numbers could be constants on top? Applies to the old implementation.
aalu1418
force-pushed
the
runlog-service-refactor-tier-3
branch
from
3df499c to
f10388c
Compare
|
Rebased it to the Notes: https://stackoverflow.com/questions/41859325/rebase-branch-after-github-squash-and-merge-onto-master |
|
@aalu1418 can you merge this along with the core_ws refactor? |
3 participants
No description provided.