fix(marketplace): catch Marketplace_SlotIsFree and continue the cancelled process #1139
Conversation
2-towns
added
Marketplace
| try: | ||
| await market.freeSlot(slot.id) | ||
| except Marketplace_SlotIsFree as e: | ||
| info "The slot cannot be freed because it is already free." |
There was a problem hiding this comment.
If this was a SP that had filled a slot, and the slot could not be freed, I would think this should be a warning. However, in other cases, this is normal behaviour. So I'd probably reduce this to a debug level because in either case, it's not essential that the user sees the log and we don't really need it to display in default Codex logging settings.
| @@ -381,8 +384,8 @@ method canReserveSlot*( | |||
| func setCanReserveSlot*(market: MockMarket, canReserveSlot: bool) = | |||
| market.canReserveSlot = canReserveSlot | |||
|
|
|||
| func setReserveSlotThrowError*(market: MockMarket, error: ?(ref MarketError)) = | |||
| market.reserveSlotThrowError = error | |||
| func setSlotThrowError*(market: MockMarket, error: ?(ref CatchableError)) = | |||
There was a problem hiding this comment.
Along the lines of the comment in the other PR, i'd make this specific to freeSlot, ie setErrorOnFreeSlot
| await market.freeSlot(slot.id) | ||
|
|
||
| let error = newException(SaleFailedError, "Sale failed") | ||
| return some State(SaleErrored(error: error)) | ||
| except CancelledError as e: | ||
| trace "SaleFailed.run was cancelled", error = e.msgDetail | ||
| except Marketplace_SlotIsFree as e: | ||
| info "The slot cannot be freed because it is already free.", error = e.msg |
There was a problem hiding this comment.
Same as above, re: info -> debug
| ) | ||
|
|
||
| let error = newException(Marketplace_SlotIsFree, "") | ||
| market.setSlotThrowError(some cast[ref CatchableError](error)) |
There was a problem hiding this comment.
Same comment in other PR regarding use of generics to prevent a conversion/cast.
| returnBytesWas = bool.none | ||
| reprocessSlotWas = bool.none | ||
| returnedCollateralValue = UInt256.none |
There was a problem hiding this comment.
This should be in setup, no?
| discard await state.run(agent) | ||
| check eventually returnBytesWas == some true | ||
| check eventually reprocessSlotWas == some false | ||
| check eventually returnedCollateralValue == some currentCollateral |
There was a problem hiding this comment.
I think maybe we can improve the assumptions we're testing here because although we call onCleanUp directly in run, we might also go to SaleErrored in run which also calls onCleanUp, and I believe this test passes because the returned collateral in onCleanUp is not the default value of none, which makes this test a bit fragile. We should instead maybe split this between two tests:
- Moves to
SaleErroredif an unexpected error is raised - Remains in
SaleCancelledif "slot not free" error is raised
The test here covers #2, but would just need to check await state.run(agent) == none State instead of checking the values of onCleanUp that were called.
Fix #1138