Add 5s timeout for market operations #305
Conversation
azawlocki
changed the title
| @@ -368,6 +368,32 @@ async def accept_payment_for_agreement(agreement_id: str, *, partial: bool = Fal | |||
| ) | |||
|
|
|||
| async def find_offers() -> None: | |||
| async def reject_proposal(proposal, reason): | |||
There was a problem hiding this comment.
hmm... the only thing I'm concerned about is that again, we're adding inner-scope functions ... whereas maybe we should start pulling that stuff outside ... but I do understand that the milk is spilled by find_offers already so it would be artificial to pull these as methods and leave find_offers as internal...
There was a problem hiding this comment.
Sometimes inner functions are just better - object-oriented programming is not the only paradigm, let's not be afraid of them. Actually, I like @azawlocki's solution very much, it's an improvement over the original code and over my PR to yajsapi, which AFAIR added another copy of try/call api/catch/emit.
There was a problem hiding this comment.
These functions are not visible outside of find_offers(). If they are not used outside, IMO they shouldn't be moved as they are "helper" functions.
There was a problem hiding this comment.
@shadeofblue I've just started pulling stuff outside in #306, I'm not sure we want to do that kind of refactoring in a PR to a release branch.
There was a problem hiding this comment.
apart from the more general remark about inner scope functions, it looks good to me
still, I'd wait for @filipgolem to have a look
| @@ -368,6 +368,32 @@ async def accept_payment_for_agreement(agreement_id: str, *, partial: bool = Fal | |||
| ) | |||
|
|
|||
| async def find_offers() -> None: | |||
| async def reject_proposal(proposal, reason): | |||
There was a problem hiding this comment.
Sometimes inner functions are just better - object-oriented programming is not the only paradigm, let's not be afraid of them. Actually, I like @azawlocki's solution very much, it's an improvement over the original code and over my PR to yajsapi, which AFAIR added another copy of try/call api/catch/emit.
| with contextlib.suppress(Exception): | ||
| await proposal.reject(reason="Score too low") | ||
| emit(events.ProposalRejected(prop_id=proposal.id, reason="Score too low")) | ||
| await reject_proposal(proposal, "Score too low") |
There was a problem hiding this comment.
This is not required, because there is no code after all if branches, but maybe we can add continue after the call to show our intent? Feel free to ignore this suggestion.
| await reject_proposal(proposal, "Score too low") | |
| await reject_proposal(proposal, "Score too low") | |
| continue |
azawlocki
changed the title
This could serve as a hotfix/work-around for #304.
For the next release we should think about making the code in
collect_offers()more concurrent, for example delegate processing of each offer to a pool 5 ofasynciotasks.