Skip to content
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.

Sign up for GitHub

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

fix(marketplace): catch Marketplace_SlotIsFree and continue the cancelled process #1139

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(marketplace): catch Marketplace_SlotIsFree and continue the cancelled process #1139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
63fe295
Catch Marketplace_SlotIsFree and continue the cancelled process
2-towns Mar 3, 2025
e8f8037
Add log message when the slot if free during failed state
2-towns Mar 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion codex/contracts/market.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@

method currentCollateral*(
market: OnChainMarket, slotId: SlotId
): Future[UInt256] {.async.} =
): Future[UInt256] {.async: (raises: [MarketError, AsyncLockError, CancelledError]).} =

Check warning on line 228 in codex/contracts/market.nim

View check run for this annotation

Codecov / codecov/patch

codex/contracts/market.nim#L228 

Added line #L228 was not covered by tests
convertEthersError:
return await market.contract.currentCollateral(slotId)

Expand Down
4 changes: 3 additions & 1 deletion codex/market.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,9 @@ method getHost*(

method currentCollateral*(
market: Market, slotId: SlotId
): Future[UInt256] {.base, async.} =
): Future[UInt256] {.
base, async: (raises: [MarketError, AsyncLockError, CancelledError])
.} =
raiseAssert("not implemented")

method getActiveSlot*(market: Market, slotId: SlotId): Future[?Slot] {.base, async.} =
Expand Down
7 changes: 6 additions & 1 deletion codex/sales/states/cancelled.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import ../../utils/exceptions
import ../salesagent
import ../statemachine
import ./errored
from ../../contracts/marketplace import Marketplace_SlotIsFree

logScope:
topics = "marketplace sales cancelled"
Expand All @@ -27,7 +28,11 @@ method run*(
debug "Collecting collateral and partial payout",
requestId = data.requestId, slotIndex = data.slotIndex
let currentCollateral = await market.currentCollateral(slot.id)
await market.freeSlot(slot.id)

try:
await market.freeSlot(slot.id)
except Marketplace_SlotIsFree as e:
info "The slot cannot be freed because it is already free."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if onClear =? agent.context.onClear and request =? data.request:
onClear(request, data.slotIndex)
Expand Down
6 changes: 6 additions & 0 deletions codex/sales/states/failed.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import ../salesagent
import ../statemachine
import ./errored
from ../../contracts/marketplace import Marketplace_SlotIsFree

logScope:
topics = "marketplace sales failed"
Expand All @@ -28,12 +29,17 @@
let slot = Slot(request: request, slotIndex: data.slotIndex)
debug "Removing slot from mySlots",
requestId = data.requestId, slotIndex = data.slotIndex

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, re: info -> debug

let error = newException(SaleFailedError, "Sale failed")
return some State(SaleErrored(error: error))

Check warning on line 42 in codex/sales/states/failed.nim

View check run for this annotation

Codecov / codecov/patch

codex/sales/states/failed.nim#L40-L42 

Added lines #L40 - L42 were not covered by tests
except CatchableError as e:
error "Error during SaleFailed.run", error = e.msgDetail
return some State(SaleErrored(error: e))
13 changes: 8 additions & 5 deletions tests/codex/helpers/mockmarket.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
subscriptions: Subscriptions
config*: MarketplaceConfig
canReserveSlot*: bool
reserveSlotThrowError*: ?(ref MarketError)
slotThrowError*: ?(ref CatchableError)
clock: ?Clock

Fulfillment* = object
Expand Down Expand Up @@ -235,7 +235,7 @@

method currentCollateral*(
market: MockMarket, slotId: SlotId
): Future[UInt256] {.async.} =
): Future[UInt256] {.async: (raises: [MarketError, AsyncLockError, CancelledError]).} =
for slot in market.filled:
if slotId == slotId(slot.requestId, slot.slotIndex):
return slot.collateral
Expand Down Expand Up @@ -311,6 +311,9 @@
market.fillSlot(requestId, slotIndex, proof, market.signer, collateral)

method freeSlot*(market: MockMarket, slotId: SlotId) {.async.} =
if error =? market.slotThrowError:
raise error

Check warning on line 316 in tests/codex/helpers/mockmarket.nim

View check run for this annotation

Codecov / codecov/patch

tests/codex/helpers/mockmarket.nim#L316 

Added line #L316 was not covered by tests
market.freed.add(slotId)
for s in market.filled:
if slotId(s.requestId, s.slotIndex) == slotId:
Expand Down Expand Up @@ -370,7 +373,7 @@
method reserveSlot*(
market: MockMarket, requestId: RequestId, slotIndex: uint64
) {.async.} =
if error =? market.reserveSlotThrowError:
if error =? market.slotThrowError:
raise error

method canReserveSlot*(
Expand All @@ -381,8 +384,8 @@
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)) =

Check warning on line 387 in tests/codex/helpers/mockmarket.nim

View check run for this annotation

Codecov / codecov/patch

tests/codex/helpers/mockmarket.nim#L387 

Added line #L387 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along the lines of the comment in the other PR, i'd make this specific to freeSlot, ie setErrorOnFreeSlot

market.slotThrowError = error

method subscribeRequests*(
market: MockMarket, callback: OnRequest
Expand Down
23 changes: 23 additions & 0 deletions tests/codex/sales/states/testcancelled.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import pkg/codex/sales/states/cancelled
import pkg/codex/sales/salesagent
import pkg/codex/sales/salescontext
import pkg/codex/market
from pkg/codex/contracts/marketplace import Marketplace_SlotIsFree

import ../../../asynctest
import ../../examples
Expand Down Expand Up @@ -40,6 +41,11 @@ asyncchecksuite "sales state 'cancelled'":
agent.onCleanUp = onCleanUp
state = SaleCancelled.new()

teardown:
returnBytesWas = bool.none
reprocessSlotWas = bool.none
returnedCollateralValue = UInt256.none
Comment on lines +45 to +47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in setup, no?


test "calls onCleanUp with returnBytes = false, reprocessSlot = true, and returnedCollateral = currentCollateral":
market.fillSlot(
requestId = request.id,
Expand All @@ -52,3 +58,20 @@ asyncchecksuite "sales state 'cancelled'":
check eventually returnBytesWas == some true
check eventually reprocessSlotWas == some false
check eventually returnedCollateralValue == some currentCollateral

test "calls onCleanUp and returns the collateral when free slot error is raised":
market.fillSlot(
requestId = request.id,
slotIndex = slotIndex,
proof = Groth16Proof.default,
host = Address.example,
collateral = currentCollateral,
)

let error = newException(Marketplace_SlotIsFree, "")
market.setSlotThrowError(some cast[ref CatchableError](error))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment in other PR regarding use of generics to prevent a conversion/cast.


discard await state.run(agent)
check eventually returnBytesWas == some true
check eventually reprocessSlotWas == some false
check eventually returnedCollateralValue == some currentCollateral
Comment on lines +74 to +77
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Moves to SaleErrored if an unexpected error is raised
  2. Remains in SaleCancelled if "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.

4 changes: 2 additions & 2 deletions tests/codex/sales/states/testslotreserving.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,15 @@ asyncchecksuite "sales state 'SlotReserving'":

test "run switches to errored when slot reservation errors":
let error = newException(MarketError, "some error")
market.setReserveSlotThrowError(some error)
market.setSlotThrowError(some cast[ref CatchableError](error))
let next = !(await state.run(agent))
check next of SaleErrored
let errored = SaleErrored(next)
check errored.error == error

test "catches reservation not allowed error":
let error = newException(MarketError, "SlotReservations_ReservationNotAllowed")
market.setReserveSlotThrowError(some error)
market.setSlotThrowError(some cast[ref CatchableError](error))
let next = !(await state.run(agent))
check next of SaleIgnored
check SaleIgnored(next).reprocessSlot == false
Expand Down
Loading