Skip to content
This repository was archived by the owner on Mar 28, 2023. It is now read-only.

Conversation

@nkuba
Copy link
Member

@nkuba nkuba commented Feb 16, 2021

Previously when an error was returned from provideBTCFundingProof transaction in the autoSubmit flow an UnhandledPromiseRejectionWarning was logged, a user of the library had no chance to handle this error.

UnhandledPromiseRejectionWarning: Error: execution reverted: not at current or previous difficulty
    at WebsocketSubprovider._handleSocketMessage (/e2e/node_modules/web3-provider-engine/subproviders/websocket.js:121:18)
    at WebSocket.onMessage (/e2e/node_modules/ws/lib/event-target.js:120:16)
    at WebSocket.emit (events.js:315:20)
    at Receiver.receiverOnMessage (/e2e/node_modules/ws/lib/websocket.js:720:20)
    at Receiver.emit (events.js:315:20)
    at Receiver.dataMessage (/e2e/node_modules/ws/lib/receiver.js:414:14)
    at Receiver.getData (/e2e/node_modules/ws/lib/receiver.js:346:17)
    at Receiver.startLoop (/e2e/node_modules/ws/lib/receiver.js:133:22)
    at Receiver._write (/e2e/node_modules/ws/lib/receiver.js:69:10)
    at writeOrBuffer (_stream_writable.js:352:12)
    at Receiver.Writable.write (_stream_writable.js:303:10)
    at TLSSocket.socketOnData (/e2e/node_modules/ws/lib/websocket.js:795:35)
    at TLSSocket.emit (events.js:315:20)
    at addChunk (_stream_readable.js:302:12)
    at readableAddChunk (_stream_readable.js:278:9)
    at TLSSocket.Readable.push (_stream_readable.js:217:10)
    at TLSWrap.onStreamRead (internal/stream_base_commons.js:186:23)

As auto submit happens asynchronously we need a way to bubble up the error.
In this proposed approach we catch the error and emit it as an event. User is required to define error callback with onError function.

The event emitter was renamed to a generic name, so it won't be
dedicated to just one type of event but it will be used for
different types of events.
Previously when an error was returned from `provideBTCFundingProof`
transaction in the `autoSubmit` flow an UnhandledPromiseRejectionWarning
was logged, user of the library had no chance to handle this error.

As autoSubmit happens asynchronously we need a way to bubble up the
error. In this proposed approach we catch the error an emit it as an
event. User is required to define error callback with `onError` function.
@nkuba nkuba requested a review from Shadowfiend February 16, 2021 12:49
}
)
})
.catch(this.notifyError)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not take an onError or similar parameter to autoSubmit? It could be optional if someone wanted to handle errors directly on the promise, and we could also emit the event.

In general, a hybrid mechanism where we move to promise workflow + events depending on what the consumer wants to use might be a nice transition for a future direction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point. Handled in #119

@Shadowfiend
Copy link
Contributor

Merging this as-is, but flagging that I'm still interested in the parameter being passed in addition to the notification approach.

@Shadowfiend Shadowfiend merged commit 2b2741c into master Mar 18, 2021
@Shadowfiend Shadowfiend deleted the deposit-emit-error branch March 18, 2021 17:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants