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

Jump to bottom

feat: expose underlying nim-ethers errors to logs #985

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+74 −23
Open

feat: expose underlying nim-ethers errors to logs #985

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
c830bda
feat: expose underlying nim-ethers errors to logs
AuHau Nov 4, 2024
8a0e3ac
chore: bump nim-ethers
AuHau Nov 29, 2024
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
7 changes: 6 additions & 1 deletion codex/contracts/clock.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import std/times
import pkg/ethers
import pkg/questionable
import pkg/chronos
import pkg/stint
import ../clock
Expand Down Expand Up @@ -45,7 +46,11 @@
if clock.started:
return

proc onBlock(_: Block) =
proc onBlock(blckResult: ?!Block) =
if eventError =? blckResult.errorOption:
error "There was an error in block subscription", msg=eventError.msg
Copy link
Contributor

Choose a reason for hiding this comment

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

Part of me thinks if the clock fails to update due to fetching a block error, then we should raise a Defect, because a correct clock is critical for proper node function. However, we don't know if it's a temporary error or not.

We could add an error count and trigger a Defect after a few errors, but that sounds potentially like an over-optimisation without data yet. Maybe for now we should add a metric and monitor it? Wdyt?

return

Check warning on line 53 in codex/contracts/clock.nim

View check run for this annotation

Codecov / codecov/patch

codex/contracts/clock.nim#L49-L53 

Added lines #L49 - L53 were not covered by tests
# ignore block parameter; hardhat may call this with pending blocks
asyncSpawn clock.update()

Expand Down
66 changes: 55 additions & 11 deletions codex/contracts/market.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,11 @@
method subscribeRequests*(market: OnChainMarket,
callback: OnRequest):
Future[MarketSubscription] {.async.} =
proc onEvent(event: StorageRequested) {.upraises:[].} =
proc onEvent(eventResult: ?!StorageRequested) {.upraises:[].} =
without event =? eventResult, eventErr:
error "There was an error in Request subscription", msg = eventErr.msg
return
Comment on lines +281 to +283
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 for most of these event subscriptions in the Market abstraction -- they are critical to node functionality, so should we log and continue or are they severe enough that we cannot assume normal node operation afterwards (thus warranting a Defect)?


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

View check run for this annotation

Codecov / codecov/patch

codex/contracts/market.nim#L280-L284 

Added lines #L280 - L284 were not covered by tests
callback(event.requestId,
event.ask,
event.expiry)
Expand All @@ -289,7 +293,11 @@
method subscribeSlotFilled*(market: OnChainMarket,
callback: OnSlotFilled):
Future[MarketSubscription] {.async.} =
proc onEvent(event: SlotFilled) {.upraises:[].} =
proc onEvent(eventResult: ?!SlotFilled) {.upraises:[].} =
without event =? eventResult, eventErr:
error "There was an error in SlotFilled subscription", msg = eventErr.msg
return

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

View check run for this annotation

Codecov / codecov/patch

codex/contracts/market.nim#L296-L300 

Added lines #L296 - L300 were not covered by tests
callback(event.requestId, event.slotIndex)

convertEthersError:
Expand All @@ -311,7 +319,11 @@
method subscribeSlotFreed*(market: OnChainMarket,
callback: OnSlotFreed):
Future[MarketSubscription] {.async.} =
proc onEvent(event: SlotFreed) {.upraises:[].} =
proc onEvent(eventResult: ?!SlotFreed) {.upraises:[].} =
without event =? eventResult, eventErr:
error "There was an error in SlotFreed subscription", msg = eventErr.msg
return

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

View check run for this annotation

Codecov / codecov/patch

codex/contracts/market.nim#L322-L326 

Added lines #L322 - L326 were not covered by tests
callback(event.requestId, event.slotIndex)

convertEthersError:
Expand All @@ -322,7 +334,11 @@
market: OnChainMarket,
callback: OnSlotReservationsFull): Future[MarketSubscription] {.async.} =

proc onEvent(event: SlotReservationsFull) {.upraises:[].} =
proc onEvent(eventResult: ?!SlotReservationsFull) {.upraises:[].} =
without event =? eventResult, eventErr:
error "There was an error in SlotReservationsFull subscription", msg = eventErr.msg
return

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

View check run for this annotation

Codecov / codecov/patch

codex/contracts/market.nim#L337-L341 

Added lines #L337 - L341 were not covered by tests
callback(event.requestId, event.slotIndex)

convertEthersError:
Expand All @@ -332,7 +348,11 @@
method subscribeFulfillment(market: OnChainMarket,
callback: OnFulfillment):
Future[MarketSubscription] {.async.} =
proc onEvent(event: RequestFulfilled) {.upraises:[].} =
proc onEvent(eventResult: ?!RequestFulfilled) {.upraises:[].} =
without event =? eventResult, eventErr:
error "There was an error in RequestFulfillment subscription", msg = eventErr.msg
return

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

View check run for this annotation

Codecov / codecov/patch

codex/contracts/market.nim#L351-L355 

Added lines #L351 - L355 were not covered by tests
callback(event.requestId)

convertEthersError:
Expand All @@ -343,7 +363,11 @@
requestId: RequestId,
callback: OnFulfillment):
Future[MarketSubscription] {.async.} =
proc onEvent(event: RequestFulfilled) {.upraises:[].} =
proc onEvent(eventResult: ?!RequestFulfilled) {.upraises:[].} =
without event =? eventResult, eventErr:
error "There was an error in RequestFulfillment subscription", msg = eventErr.msg
return

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

View check run for this annotation

Codecov / codecov/patch

codex/contracts/market.nim#L366-L370 

Added lines #L366 - L370 were not covered by tests
if event.requestId == requestId:
callback(event.requestId)

Expand All @@ -354,7 +378,11 @@
method subscribeRequestCancelled*(market: OnChainMarket,
callback: OnRequestCancelled):
Future[MarketSubscription] {.async.} =
proc onEvent(event: RequestCancelled) {.upraises:[].} =
proc onEvent(eventResult: ?!RequestCancelled) {.upraises:[].} =
without event =? eventResult, eventErr:
error "There was an error in RequestCancelled subscription", msg = eventErr.msg
return

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

View check run for this annotation

Codecov / codecov/patch

codex/contracts/market.nim#L381-L385 

Added lines #L381 - L385 were not covered by tests
callback(event.requestId)

convertEthersError:
Expand All @@ -365,7 +393,11 @@
requestId: RequestId,
callback: OnRequestCancelled):
Future[MarketSubscription] {.async.} =
proc onEvent(event: RequestCancelled) {.upraises:[].} =
proc onEvent(eventResult: ?!RequestCancelled) {.upraises:[].} =
without event =? eventResult, eventErr:
error "There was an error in RequestCancelled subscription", msg = eventErr.msg
return

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

View check run for this annotation

Codecov / codecov/patch

codex/contracts/market.nim#L396-L400 

Added lines #L396 - L400 were not covered by tests
if event.requestId == requestId:
callback(event.requestId)

Expand All @@ -376,7 +408,11 @@
method subscribeRequestFailed*(market: OnChainMarket,
callback: OnRequestFailed):
Future[MarketSubscription] {.async.} =
proc onEvent(event: RequestFailed) {.upraises:[]} =
proc onEvent(eventResult: ?!RequestFailed) {.upraises:[]} =
without event =? eventResult, eventErr:
error "There was an error in RequestFailed subscription", msg = eventErr.msg
return

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

View check run for this annotation

Codecov / codecov/patch

codex/contracts/market.nim#L411-L415 

Added lines #L411 - L415 were not covered by tests
callback(event.requestId)

convertEthersError:
Expand All @@ -387,7 +423,11 @@
requestId: RequestId,
callback: OnRequestFailed):
Future[MarketSubscription] {.async.} =
proc onEvent(event: RequestFailed) {.upraises:[]} =
proc onEvent(eventResult: ?!RequestFailed) {.upraises:[]} =
without event =? eventResult, eventErr:
error "There was an error in RequestFailed subscription", msg = eventErr.msg
return

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

View check run for this annotation

Codecov / codecov/patch

codex/contracts/market.nim#L426-L430 

Added lines #L426 - L430 were not covered by tests
if event.requestId == requestId:
callback(event.requestId)

Expand All @@ -398,7 +438,11 @@
method subscribeProofSubmission*(market: OnChainMarket,
callback: OnProofSubmitted):
Future[MarketSubscription] {.async.} =
proc onEvent(event: ProofSubmitted) {.upraises: [].} =
proc onEvent(eventResult: ?!ProofSubmitted) {.upraises: [].} =
without event =? eventResult, eventErr:
error "There was an error in ProofSubmitted subscription", msg = eventErr.msg
return

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

View check run for this annotation

Codecov / codecov/patch

codex/contracts/market.nim#L441-L445 

Added lines #L441 - L445 were not covered by tests
callback(event.id)

convertEthersError:
Expand Down
5 changes: 3 additions & 2 deletions tests/integration/testecbug.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ marketplacesuite "Bug #821 - node crashes during erasure coding":
let cid = clientApi.upload(data).get

var requestId = none RequestId
proc onStorageRequested(event: StorageRequested) {.raises:[].} =
requestId = event.requestId.some
proc onStorageRequested(eventRes: ?!StorageRequested)=
if event =? eventRes:
requestId = event.requestId.some
Comment on lines +36 to +38
Copy link
Contributor

Choose a reason for hiding this comment

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

Wdyt about raising a Defect (and fail the test) if there's an error in the callback?

Suggested change
proc onStorageRequested(eventRes: ?!StorageRequested)=
if event =? eventRes:
requestId = event.requestId.some
proc onStorageRequested(event: ?!StorageRequested)=
requestId = some event.get.requestId


let subscription = await marketplace.subscribe(StorageRequested, onStorageRequested)

Expand Down
5 changes: 3 additions & 2 deletions tests/integration/testmarketplace.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,9 @@ marketplacesuite "Marketplace payouts":
let cid = clientApi.upload(data).get

var slotIdxFilled = none UInt256
proc onSlotFilled(event: SlotFilled) =
slotIdxFilled = some event.slotIndex
proc onSlotFilled(eventRes: ?!SlotFilled) =
if event =? eventRes:
slotIdxFilled = some event.slotIndex
Comment on lines +138 to +140
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here...

Suggested change
proc onSlotFilled(eventRes: ?!SlotFilled) =
if event =? eventRes:
slotIdxFilled = some event.slotIndex
proc onSlotFilled(event: ?!SlotFilled) =
slotIdxFilled = some event.get.slotIndex


let subscription = await marketplace.subscribe(SlotFilled, onSlotFilled)

Expand Down
12 changes: 6 additions & 6 deletions tests/integration/testproofs.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ marketplacesuite "Hosts submit regular proofs":
check eventually(client0.purchaseStateIs(purchaseId, "started"), timeout = expiry.int * 1000)

var proofWasSubmitted = false
proc onProofSubmitted(event: ProofSubmitted) =
proofWasSubmitted = true
proc onProofSubmitted(event: ?!ProofSubmitted) =
proofWasSubmitted = event.isOk

let subscription = await marketplace.subscribe(ProofSubmitted, onProofSubmitted)

Expand Down Expand Up @@ -120,8 +120,8 @@ marketplacesuite "Simulate invalid proofs":
check eventually(client0.purchaseStateIs(purchaseId, "started"), timeout = expiry.int * 1000)

var slotWasFreed = false
proc onSlotFreed(event: SlotFreed) =
if event.requestId == requestId:
proc onSlotFreed(event: ?!SlotFreed) =
if event.isOk and event.value.requestId == requestId:
slotWasFreed = true

let subscription = await marketplace.subscribe(SlotFreed, onSlotFreed)
Expand Down Expand Up @@ -185,8 +185,8 @@ marketplacesuite "Simulate invalid proofs":
check eventually(slotWasFilled, timeout = expiry.int * 1000)

var slotWasFreed = false
proc onSlotFreed(event: SlotFreed) =
if event.requestId == requestId:
proc onSlotFreed(event: ?!SlotFreed) =
if event.isOk and event.value.requestId == requestId:
slotWasFreed = true
let freedSubscription = await marketplace.subscribe(SlotFreed, onSlotFreed)

Expand Down
2 changes: 1 addition & 1 deletion vendor/nim-ethers
Open in desktop
Submodule nim-ethers updated from 1ae2cd to 2808a0
Loading