-
Notifications
You must be signed in to change notification settings - Fork 56
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(solver/app): basic event processor #2386
Conversation
62f8f91
to
c0c6198
Compare
c0c6198
to
e070b39
Compare
req, _, err := deps.GetRequest(ctx, chainID, reqID) | ||
if err != nil { | ||
return errors.Wrap(err, "current status") | ||
} else if event.Status != req.Status { |
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.
does it make sense to also check for case where event status is further than request status and mark it as a bug? so event status is fullffilled but request status is pending, because if this happens (if it can) we might miss a bug 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.
yeah, good point, I'll add this in next PR
ignored = "" | ||
) | ||
|
||
func TestEventProcessor(t *testing.T) { |
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.
nice
) | ||
|
||
var ( | ||
inboxABI = mustGetABI(bindings.SolveInboxMetaData) |
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.
nit: use the Golang init function pattern instead of mustGet*
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.
init
should be avoided at all costs I think
) | ||
|
||
// procDeps abstracts dependencies for the event processor allowed simplified testing. | ||
type procDeps struct { |
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.
nit: this should be an interface, interface is used to do exactly what the comment says "abstracts dependencies for ... allowed simplified testing"
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.
it is a style thing, I prefer funcs, since the implementations are not related, this allows better decoupling,
return deps.Reject(ctx, chainID, req, reason) | ||
} | ||
|
||
return deps.Accept(ctx, chainID, req) |
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.
Can you explain why we don't Fulfill at this step?
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.
First Accept, if that succeeds, then fulfil
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.
Couple nits related to Go styling, and one comment for clarification.
Basic solver architecture based on a well-abstracted event processor.
issue: #2384