-
Notifications
You must be signed in to change notification settings - Fork 144
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
Market actor update #593
Market actor update #593
Conversation
actor_error!(ErrIllegalState; | ||
"failed to get deal_id ({}): {}", deal_id, e) | ||
})? | ||
.ok_or_else(|| actor_error!(ErrNotFound; "no such deal_id: {}", deal_id))?; |
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 don't see them using the ErrNotFound
actor error here. Understandably we need to handle this error but should it be an actor error?
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 this ones a bit harder to trace, it's within the getDealProposal
function, the !found
check. It was counterproductive to add that function and check a downcasted Box<dyn StdError>
outside of it. (if this is still confusing I can explain in more detail)
.map_err( | ||
|e| actor_error!(ErrIllegalState; "failed to get deal_id ({}): {}", deal_id, e), | ||
)? | ||
.ok_or_else(|| actor_error!(ErrNotFound; "proposal doesn't exist ({})", deal_id))?; |
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.
Similar question here as above.
Summary of changes
Changes introduced in this pull request:
The changes overlapping with the miner and power actor refactors I tried to leave out of this PR, so those actors will be pretty broken until I get around to them
Also all of the market tests broke with the protocol changes and there is now 3000 lines of code in the tests so I'm not doing in this PR
Reference issue to close (if applicable)
Closes #558
Other information and links