-
Notifications
You must be signed in to change notification settings - Fork 59
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
Include chain height when running custom decision logic #322
Conversation
a190687
to
3eb2fa9
Compare
Codecov Report
@@ Coverage Diff @@
## master #322 +/- ##
==========================================
+ Coverage 63.62% 63.67% +0.06%
==========================================
Files 41 41
Lines 2608 2601 -7
==========================================
- Hits 1659 1656 -3
+ Misses 827 823 -4
Partials 122 122
Continue to review full report at Codecov.
|
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.
I'm ok with this in general but it seems like the deal decider, which is running in the node, should have its own access to chain height, which we get from the node to begin with.
if err != nil { | ||
return ctx.Trigger(storagemarket.ProviderEventDealRejected, xerrors.Errorf("custom deal decision logic failed to get chain head: %w", err)) | ||
} | ||
accept, reason, err := environment.RunCustomDecisionLogic(ctx.Context(), deal, ht) |
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.
Shouldn't whomever is implementing the deal decider have access to chain height on their own? We only get it from the node to begin with. My inclination would be to assume the deal decider can get access to whatever Chain APIs are needed to determine chain height.
3eb2fa9
to
4383454
Compare
Please ignore docs-check I'm not sure why it's failing! |
I'm not going to rush out a new release since I think your PR no longer needs this. |
Subsumed by filecoin-project/lotus#2384