feat: Websocket connection to the Atomic Swap Service #533

tuliomir · 2023-05-19T16:33:34Z

Acceptance Criteria

Establishes the connection with the Atomic Swap Service via a websocket protocol

Note: The subscribing and unsubscribing of proposals will be made at a future PR. This one interacts with the server foundation provided by the backend on HathorNetwork/hathor-atomic-swap-service#5

Security Checklist

Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

codecov · 2023-05-19T16:42:08Z

Codecov Report

Merging #533 (f205976) into dev (e0c9926) will increase coverage by 0.09%.
The diff coverage is 91.17%.

@@            Coverage Diff             @@
##              dev     #533      +/-   ##
==========================================
+ Coverage   77.57%   77.67%   +0.09%     
==========================================
  Files          66       67       +1     
  Lines        5173     5205      +32     
  Branches     1094     1101       +7     
==========================================
+ Hits         4013     4043      +30     
- Misses       1149     1151       +2     
  Partials       11       11

Impacted Files	Coverage Δ
src/connection.ts	`70.58% <ø> (ø)`
src/swapService/swapConnection.ts	`90.32% <90.32%> (ø)`
src/new/connection.ts	`90.32% <100.00%> (ø)`
src/websocket/index.ts	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

r4mmer · 2023-05-24T18:17:57Z

src/new/swapConnection.ts

+ * You can subscribe for the following events:
+ * - update-atomic-swap-proposal: Fired when the state of a listened proposal changes
+ **/
+export class AtomicSwapServiceConnection extends BaseConnection {


Why are we extending BaseConnection when BaseConnection was meant to be a ws connection with the fullnode?

Good point! Opened #535 to discuss the refactoring possibilities for this, including the Wallet Service websocket that is already implemented using it.

I agree with @r4mmer that it's a different use. The BaseConnection is not meant to be used with full node only but with wallet context.

I think it's bad to use this as base class, you should refactor it before merging, it's weird that you have your ConnectionParams options with network and servers, which makes no sense for the swap service.

Refactored the AtomicSwapServiceConnection class to extend directly from EventEmitter on f440e3c.

r4mmer · 2023-05-24T18:22:51Z

src/websocket/atomic-swap.ts

+   */
+  onMessage(evt) {
+    const message = JSON.parse(evt.data)
+    const _type = message.type.split(':')[0]


Why are we splitting the type with :?

It wasn't necessary. Removed on d1d252f

pedroferreira1 · 2023-06-01T03:18:48Z

src/websocket/atomic-swap.ts

+  /**
+   * Returns a JSON stringified ping message
+   */
+  getPingMessage() {


Do you have a pong in the swap service ws?

Yes, we do.

pedroferreira1 · 2023-06-01T03:21:34Z

src/websocket/atomic-swap.ts

+ * @class
+ * @name AtomicSwapWebSocket
+ */
+class AtomicSwapWebSocket extends BaseWebSocket {


Is there any difference between this class and WalletWebSocket? The only one I've seen is the type split, which could be a parameter.

Good point! I refactored all my code to use the WalletWebSocket instead, even the tests.
And since this class no longer serves only the wallet, I renamed it to GenericWebSocket on bb5f334.

Do you agree with these changes?

pedroferreira1 · 2023-06-01T03:23:20Z

src/new/swapConnection.ts

+ * You can subscribe for the following events:
+ * - update-atomic-swap-proposal: Fired when the state of a listened proposal changes
+ **/
+export class AtomicSwapServiceConnection extends BaseConnection {


I agree with @r4mmer that it's a different use. The BaseConnection is not meant to be used with full node only but with wallet context.

I think it's bad to use this as base class, you should refactor it before merging, it's weird that you have your ConnectionParams options with network and servers, which makes no sense for the swap service.

pedroferreira1 · 2023-06-01T03:24:08Z

src/new/swapConnection.ts

@@ -0,0 +1,95 @@
+/**


I don't think the folder new is the right folder for it. I would create a new folder for swapService, or something like that, to keep the swap service files.

Indeed, the scope of this service is greater than the one on the new folder.
Moved it to /src/swapService on 13bceac.

AtomicSwapServiceConnection no longer extends BaseConnection

pedroferreira1 · 2023-06-09T19:08:51Z

__tests__/new/swapConnection.test.ts

@@ -0,0 +1,108 @@
+import { AtomicSwapServiceConnection } from '../../src/swapService/swapConnection';


The folder of the test should change as well?

Fixed on b6207ef

pedroferreira1 · 2023-06-09T19:31:43Z

src/swapService/swapConnection.ts

+
+  unsubscribeProposal(proposalId: string) {
+    if (this.websocket) {
+      const msg = JSON.stringify({type: 'unsubscribe_proposal', proposalId});


Suggested change

const msg = JSON.stringify({type: 'unsubscribe_proposal', proposalId});

const msg = JSON.stringify({ type: 'unsubscribe_proposal', proposalId });

Fixed on f205976

tuliomir added 2 commits May 19, 2023 13:31

feat: swapConnection

7c24a43

style: code cleanup

06bf0a8

tuliomir added the enhancement New feature or request label May 19, 2023

tuliomir requested a review from r4mmer May 19, 2023 16:33

tuliomir self-assigned this May 19, 2023

tuliomir marked this pull request as ready for review May 19, 2023 20:14

tuliomir requested a review from pedroferreira1 as a code owner May 19, 2023 20:14

tuliomir marked this pull request as draft May 19, 2023 20:14

tuliomir added 5 commits May 19, 2023 20:04

test: start tests

57ff25b

test: proposal tests

e4227ea

test: increase coverage on swapConnection

4949f64

test: onOpen and ping message

6dfc315

test: onMessage

a50e3ff

tuliomir marked this pull request as ready for review May 20, 2023 00:45

docs: docstrings cleanup

aa5dcde

r4mmer reviewed May 24, 2023

View reviewed changes

tuliomir mentioned this pull request May 29, 2023

[Refactor] BaseConnection method extraction #535

Open

fix: removes unnecessary split

d1d252f

tuliomir requested a review from r4mmer May 30, 2023 14:24

r4mmer approved these changes May 31, 2023

View reviewed changes

pedroferreira1 reviewed Jun 1, 2023

View reviewed changes

tuliomir added 5 commits June 6, 2023 21:52

fix: removes unnecessary dependency

f440e3c

AtomicSwapServiceConnection no longer extends BaseConnection

refactor: removes AtomicSwapWebSocket

7e2c707

refactor: renames WalletWebSocket

bb5f334

refactor: moves swapConnection file

13bceac

Merge branch 'dev' into feat/ws-atomic-swap

adc0e79

tuliomir requested review from r4mmer and pedroferreira1 June 7, 2023 15:41

pedroferreira1 approved these changes Jun 9, 2023

View reviewed changes

tuliomir added 2 commits June 9, 2023 16:39

refactor: moves swapConnection test file

b6207ef

style: lint

f205976

r4mmer approved these changes Jun 9, 2023

View reviewed changes

tuliomir merged commit 25dcebb into dev Jun 9, 2023

tuliomir deleted the feat/ws-atomic-swap branch June 9, 2023 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Websocket connection to the Atomic Swap Service #533

feat: Websocket connection to the Atomic Swap Service #533

tuliomir commented May 19, 2023 •

edited

Loading

codecov bot commented May 19, 2023 •

edited

Loading

r4mmer May 24, 2023

tuliomir May 29, 2023

pedroferreira1 Jun 1, 2023

tuliomir Jun 7, 2023

r4mmer May 24, 2023

tuliomir May 30, 2023

pedroferreira1 Jun 1, 2023

tuliomir Jun 1, 2023

pedroferreira1 Jun 1, 2023

tuliomir Jun 7, 2023

pedroferreira1 Jun 1, 2023

pedroferreira1 Jun 1, 2023

tuliomir Jun 7, 2023

pedroferreira1 Jun 9, 2023

tuliomir Jun 9, 2023

pedroferreira1 Jun 9, 2023

tuliomir Jun 9, 2023

		@@ -0,0 +1,108 @@
		import { AtomicSwapServiceConnection } from '../../src/swapService/swapConnection';

	const msg = JSON.stringify({type: 'unsubscribe_proposal', proposalId});
	const msg = JSON.stringify({ type: 'unsubscribe_proposal', proposalId });

feat: Websocket connection to the Atomic Swap Service #533

feat: Websocket connection to the Atomic Swap Service #533

Conversation

tuliomir commented May 19, 2023 • edited Loading

Acceptance Criteria

Security Checklist

codecov bot commented May 19, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuliomir commented May 19, 2023 •

edited

Loading

codecov bot commented May 19, 2023 •

edited

Loading