Skip to content

Implement MiddlewareSubscriber #1805

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.

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

Merged
merged 1 commit into from
Jun 5, 2023
Merged

Implement MiddlewareSubscriber #1805

merged 1 commit into from
Jun 5, 2023

Conversation

davidyuk
Copy link
Member

@davidyuk davidyuk commented May 2, 2023

related to #442

I had an idea to integrate https://www.npmjs.com/package/retry to automatically reconnect in case WS gets disconnected for whatever reason, but this would be unreliable since it is not possible to get emitted events since the connection is lost. So a reconnection should be implemented on aepp side.

Currently, I'm keeping middleware responses as they are. Would be better to reuse types from Node/swagger and numbers replace with bigints to don't lose precision aeternity/ae_mdw#1301
Ideally, middleware can provide encoded transactions and I would unpack them using transaction builder aeternity/ae_mdw#1187

Other than the current approach I've considered implementing it as:

class MiddlewareWebSocket extends WebSocket {

It was rejected because it is not possible to establish the connection on demand, WebSocket can't be reconnected.

Also, I've tried to use an existing EventEmitter class:

class MiddlewareSubscriber extends EventEmitter {
...
}

subscriber.on('transaction', <handler>);
subscriber.off('transaction', <handler>);
subscriber.on('microBlock:node', <handler>);

it was fine until l came to subscriber.on('ak_...:node', <handler>). There is no easy way in TS to append :node to ak_{string} having ak_{string}:node as the resulting type. Also doing this way the event emitter would ignore incorrect targets (technically, an arbitrary event name can be used), which obviously may appear if accepting the user input or so.

EventEmitter from Node can be replaced with EventTarget from browser js, but it has the same flaws and is more verbose.

This PR is supported by the Æternity Crypto Foundation

@davidyuk davidyuk added this to the next milestone May 2, 2023
@davidyuk davidyuk force-pushed the develop branch 3 times, most recently from 1948f5f to 68edc25 Compare May 9, 2023 07:00
@davidyuk davidyuk force-pushed the feature/middleware branch from 2b68ceb to 715a767 Compare May 22, 2023 10:34
@davidyuk davidyuk changed the title Implement MiddlewareWebsocket Implement MiddlewareSubscriber May 22, 2023
"json-bigint": "^1.0.0",
"process": "^0.11.10",
"rlp": "^3.0.0",
"sha.js": "^2.4.11",
"tweetnacl": "^1.0.3",
"tweetnacl-auth": "^1.0.1",
"varuint-bitcoin": "^1.1.2",
"websocket": "^1.0.34"
"websocket": "^1.0.34",
"ws": "^8.13.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

This package seems to be much more popular than websocket, it implements the same W3C api, I would migrate state channels to it later.
https://www.npmjs.com/package/ws
https://www.npmjs.com/package/websocket

@@ -43,6 +43,11 @@ export { default as AccountLedgerFactory } from './account/LedgerFactory';
export { default as CompilerBase } from './contract/compiler/Base';
export { default as CompilerHttp } from './contract/compiler/Http';
export { default as Channel } from './channel/Contract';
export {
default as _MiddlewareSubscriber,
Copy link
Member Author

Choose a reason for hiding this comment

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

_ means that this API is not stable and going to be changed.

@davidyuk davidyuk force-pushed the feature/middleware branch from 715a767 to 4771da1 Compare May 22, 2023 11:10
@davidyuk davidyuk requested a review from thepiwo May 22, 2023 11:10
@davidyuk davidyuk force-pushed the feature/middleware branch from 4771da1 to 54380d0 Compare May 22, 2023 12:00
@davidyuk davidyuk marked this pull request as ready for review May 22, 2023 12:00
@davidyuk davidyuk force-pushed the feature/middleware branch from 54380d0 to f77b35b Compare May 22, 2023 12:29
@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

Patch coverage: 73.68% and project coverage change: -0.29 ⚠️

Comparison is base (3653b13) 82.68% compared to head (b51b0a3) 82.40%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1805      +/-   ##
===========================================
- Coverage    82.68%   82.40%   -0.29%     
===========================================
  Files           91       92       +1     
  Lines         3119     3194      +75     
  Branches       617      635      +18     
===========================================
+ Hits          2579     2632      +53     
- Misses         249      259      +10     
- Partials       291      303      +12     
Impacted Files Coverage Δ
src/MiddlewareSubscriber.ts 72.97% <72.97%> (ø)
src/channel/Base.ts 76.47% <100.00%> (ø)
src/index-browser.ts 97.56% <100.00%> (-2.44%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@thepiwo thepiwo left a comment

Choose a reason for hiding this comment

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

probably fine, didn't check in detail

@davidyuk davidyuk force-pushed the feature/middleware branch 2 times, most recently from a71ebe8 to d9c11a5 Compare June 5, 2023 05:57
@davidyuk davidyuk force-pushed the feature/middleware branch from d9c11a5 to b51b0a3 Compare June 5, 2023 06:14
@davidyuk davidyuk merged commit 14099b1 into develop Jun 5, 2023
@davidyuk davidyuk deleted the feature/middleware branch June 5, 2023 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants