feat: subscriptions get passed result questionable #91
Conversation
AuHau
force-pushed
the
feat/subscriptions-get-passed-result-questionable
branch
from
bf3c197
to
26ed902
Compare
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
| pull_request: | ||
| workflow_dispatch: |
There was a problem hiding this comment.
Why did you update the CI workflow?
There was a problem hiding this comment.
Because in the way we have it till now the CI gets "double triggered" because of PR and "push". See: https://discord.com/channels/895609329053474826/963349623714504784/1301528920893558855
| SolidityError* = object of EthersError | ||
| ContractError* = object of EthersError | ||
| SignerError* = object of EthersError | ||
| SubscriptionError* = object of EthersError | ||
| ProviderError* = object of EthersError | ||
| data*: ?seq[byte] | ||
|
|
||
| template raiseSignerError*(message: string, parent: ref ProviderError = nil) = | ||
| raise newException(SignerError, message, parent) |
There was a problem hiding this comment.
🟡 I don't really feel like these need to be in a "shared" module, unless the errors are shared across modules. Iow, I don't mind that they live in their specific module, eg SignerError in the signer module. Matter of preference maybe.
| onBlock(failure(Block, error.toErr(SubscriptionError))) | ||
| return |
There was a problem hiding this comment.
Looks like there's too many spaces here.
| for change in await getChanges(id): | ||
| callback(id, success(change)) | ||
| except CancelledError as e: | ||
| raise e |
| try: | ||
| if not id.isNil and id in subscriptions.callbacks: | ||
| subscriptions.callbacks[id].some | ||
| try: | ||
| return subscriptions.callbacks[id].some | ||
| except: discard | ||
| else: | ||
| SubscriptionCallback.none | ||
| return SubscriptionCallback.none | ||
| except KeyError: | ||
| SubscriptionCallback.none | ||
| return SubscriptionCallback.none |
There was a problem hiding this comment.
spacing lines 84-86
What exception was leaking from subscriptions.callbacks[id].some?
By default, SubscriptionCallback.none is returned, so we don't really need line 87-88.
The catching of KeyError is not needed with the try/except since it's caught on 86. If you're going to use try/except discard, then this whole thing can be a bit nicer:
if not id.isNil and id in subscriptions.callbacks:
try:
return subscriptions.callbacks[id].some
except: discard|
|
||
| proc poll(id: JsonNode) {.async: (raises: [CancelledError, SubscriptionError]).} = | ||
| without callback =? subscriptions.getCallback(id): | ||
| return |
| let wrappedErr = newException(SubscriptionError, "HTTP polling: There was an exception while getting subscription's block: " & e.msg, e) | ||
| onBlock(failure(Block, wrappedErr)) |
There was a problem hiding this comment.
🟡 can also do:
| let wrappedErr = newException(SubscriptionError, "HTTP polling: There was an exception while getting subscription's block: " & e.msg, e) | |
| onBlock(failure(Block, wrappedErr)) | |
| let err = e.toErr(SubscriptionError, | |
| "HTTP polling: There was an exception while getting subscription's block: " & e.msg) | |
| onBlock(failure(Block, err)) |
| onBlock(failure(Block, error.toErr(SubscriptionError))) | ||
| return |
| onLog(failure(Log, error.toErr(SubscriptionError))) | ||
| return |
Co-authored-by: Eric <5089238+emizzle@users.noreply.github.com>
Introduces breaking change to API, where subscription handlers now return Result type. Handlers, similarly to now will be called with events wrapped in the Result when new events are detected, but also handlers will be called when some error in the underlying event detection mechanism is detected.