feat: external notifications #237

r4mmer · 2022-09-07T17:23:13Z

Acceptance Criteria

Implement the design [Design] external notifications #236

Description

Some refactors were made to make the plugin system more reliable.
The express app configuration was moved from index.js to app.js.
This was made so that we can startup the plugins and check the configuration before start listening for requests.
This can help with health and status checks, also it ensures the plugins can listen to all events.

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 · 2022-09-07T17:34:50Z

Codecov Report

Merging #237 (31b646b) into dev (6abb754) will decrease coverage by 0.31%.
The diff coverage is 82.19%.

@@            Coverage Diff             @@
##              dev     #237      +/-   ##
==========================================
- Coverage   85.19%   84.87%   -0.32%     
==========================================
  Files          24       31       +7     
  Lines         905     1071     +166     
  Branches      185      216      +31     
==========================================
+ Hits          771      909     +138     
- Misses        116      138      +22     
- Partials       18       24       +6

Impacted Files	Coverage Δ
src/helpers/validations.helper.js	`100.00% <ø> (ø)`
src/plugins/child.js	`72.41% <72.41%> (ø)`
src/services/notification.service.js	`72.72% <72.72%> (ø)`
src/app.js	`80.95% <80.95%> (ø)`
src/helpers/notification.helper.js	`83.33% <83.33%> (ø)`
src/plugins/hathor_sqs.js	`88.23% <88.23%> (ø)`
src/controllers/index.controller.js	`79.56% <100.00%> (+0.44%)`	⬆️
src/plugins/hathor_debug.js	`100.00% <100.00%> (ø)`
src/plugins/hathor_rabbitmq.js	`100.00% <100.00%> (ø)`
src/plugins/hathor_websocket.js	`100.00% <100.00%> (ø)`
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

r4mmer · 2022-09-21T13:42:17Z

FYI for reviewers

If you want to test the plugins there may be some setup needed.
I've listed below a base for configurations i've used.

Running the plugins locally

If the wallet headless is runing with docker, remember to enable the plugins with --enabled_plugins or with the envvar HEADLESS_ENABLED_PLUGINS and the value is a space separated list of plugins to run.

debug plugin

# Config options
# `plugin_debug_long`: all, off
npm start -- --plugin_debug_long all

This plugin requires no external tools.
Just start the service and check the logs.

websocket plugin

npm start -- --plugin_ws_port 8008

We can omit the port config since the default port is 8008.

We need a websocket client or a script to test this.
The wscat works extremely well for this.

wscat -c localhost:8008

sqs

# `plugin_sqs_region`
# `plugin_sqs_queue_url`
# `plugin_sqs_endpoint_url`
npm start -- --plugin_sqs_endpoint_url http://localhost:9324 --plugin_sqs_queue_url http://localhost:9324/000000000000/queue1 --plugin_sqs_region elasticmq

To avoid having to spin up an sqs queue for dev/testing we can use ElasticMQ
that has an AWS SQS compatible API, meaning it works with aws sdk and even aws cli.
To run it, we use docker-compose.

version: "2"
services:
  queue:
    image: softwaremill/elasticmq:latest
    ports:
      - 9324:9324
      - 9325:9325
    volumes:
      - ./elasticmq.conf:/opt/elasticmq.conf

Also, we use the file elasticmq.conf

include classpath("application.conf")

node-address {
  protocol = http
  host = "*"
  port = 9324
  context-path = ""
}

rest-sqs {
  enabled = true
  bind-port = 9324
  bind-hostname = "0.0.0.0"
  # Possible values: relaxed, strict
  sqs-limits = strict
}

rest-stats {
  enabled = true
  bind-port = 9325
  bind-hostname = "0.0.0.0"
}

generate-node-address = false

queues {
  queue1 {
    defaultVisibilityTimeout = 30 seconds
    delay = 5 seconds
    receiveMessageWait = 0 seconds
    fifo = false
    contentBasedDeduplication = false
    tags {
      tag1 = "tagged1"
      tag2 = "tagged2"
    }
  }
}

# Region and accountId which will be included in resource ids
aws {
  region = us-east-1
  accountId = 000000000000
}

This will start a stats dashboard running on port 9325 which we can use to check the incomming messages.
It also starts the elasticmq instance on port 9324 with a queue named queue1.
We can check that it is running and the queue was created with:

aws --endpoint-url http://localhost:9324 sqs list-queues

While we have enough for testing, I've also created a consumer to the queue, to avoid increasing the number of unread messages on queue, and to check the contents of the messages being sent.

from time import sleep
import boto3

endpoint_url = "http://localhost:9324"

# Create SQS client
sqs = boto3.client('sqs', endpoint_url=endpoint_url, region_name='elasticmq', use_ssl=False)

queue_url = 'http://localhost:9324/000000000000/queue1'

def receive_msg():
    # Receive message from SQS queue
    response = sqs.receive_message(
        QueueUrl=queue_url,
        AttributeNames=['SentTimestamp'],
        MaxNumberOfMessages=1,
        MessageAttributeNames=['All'],
        VisibilityTimeout=0,
        WaitTimeSeconds=0
    )
    message = response['Messages'][0]
    receipt_handle = message['ReceiptHandle']
    # Delete received message from queue
    sqs.delete_message(QueueUrl=queue_url,ReceiptHandle=receipt_handle)
    print('Received and deleted message: %s' % message)

while True:
    receive_msg()
    sleep(1)

rabbitMQ plugin

# `plugin_rabbitmq_url`
# `plugin_rabbitmq_queue`
npm start -- --plugin_rabbitmq_url amqp://localhost --plugin_rabbitmq_queue queue1

We can run rabbitMQ with docker with the command:

docker run -it --rm --name rabbitmq -p 5672:5672 -p 15672:15672 rabbitmq:3.10-management

And to check the messages we can use the consumer script

var amqp = require('amqplib/callback_api');

var queue = 'amqp://localhost';

amqp.connect(queue, function(error0, connection) {
    if (error0) {
        throw error0;
    }
    connection.createChannel(function(error1, channel) {
        if (error1) {
            throw error1;
        }
        var queue = 'queue1';
        channel.assertQueue(queue, { durable: false });
        console.log(" [*] Waiting for messages in %s. To exit press CTRL+C", queue);
        channel.consume(queue, function(msg) {
            console.log(" [x] Received %s", msg.content.toString());
        }, { noAck: true });
    });
});

config.js.template

src/index.js

src/plugins/hathor_debug.js

src/plugins/hathor_rabbitmq.js

tuliomir · 2022-09-26T22:49:43Z

src/services/notification.service.js

+
+const eventBus = 'message';
+
+const WalletEventMap = {


Since the intention is to make a map and help the user avoid code mistakes, I think it would be better to have the key names in camelCase, without the quotes: state, newTx, updateTx.

While I like the idea, this would remove the original event type from the map.
For instance, if we had an event "foo-bar" which we translate to fooBar: 'wallet:foo' a dev searching for instances of the event "foo-bar" may miss this reference.

What do you think of this?
Obs: my opinion is that i would still change to camel case, i just want to show the full info on why the original event name is there.

I agree with you that the search by event name is the main way of finding the interactions with these events now, and that it's important to keep is searchable like this. As a short-term solution maybe just add the original event name as a comment?

const ConnectionEventMap = { state: 'conn:state', // original lib event: 'state' newTx: 'wallet:new-tx', // original lib event: 'new-tx' updateTx: 'wallet:update-tx', // original lib event: 'update-tx'

Do you agree that this helps avoid confusion?

--
As a more definitive solution, I would expand my suggestion to also refactor the way events are referred on the project and on the wallet lib itself:

// On wallet-lib/src/constants.js , add the following object: EVENT_NAMES: { STATE: 'state', NEW_TX: 'new-tx', UPDATE_TX: 'update-tx', } // On headless/src/services/notification.service/js , simply re-use the same key names as the lib's constants object const WalletEventMap = { STATE: 'wallet:state', NEW_TX: 'wallet:new-tx', UPDATE_TX: 'wallet:update-tx', } // The event listeners would then have the format: import { EVENT_NAMES as LIB_EVENTS } from '@hathor/wallet-lib/constants' hwallet.on(LIB_EVENTS.NEW_TX, tx => { const type = WalletEventMap.NEW_TX; ... }

This seems material for another issue / PR on both projects, as I think it would make it simpler for developers to also use the lib's events beyond the external notifications.

Created an issue to track this suggestion.

src/controllers/index.controller.js

src/child.js

src/services/notification.service.js

tuliomir · 2022-09-27T00:23:31Z

src/plugins/hathor_websocket.js

+}
+
+/* istanbul ignore next */
+export const init = async bus => {


I feel this project needs at least one integration test, and maybe the websocket plugin could be well suited for that.

A test suite containing the startup of a wallet, the real-time notification of transactions and a successful shutdown would be great also to serve as an example code to developers.

What do you think?

Besides the integration test, I think a comment inside the code pointing to the Running the plugins locally section of your comment would be great. One suggestion of a place to add it would be

hathor-wallet-headless/src/controllers/index.controller.js

Lines 182 to 183 in 50dd3a3

// XXX: subscribe to wallet events with notificationBus

notificationBus.subscribeHathorWallet(req.body['wallet-id'], wallet);

// @see https://github.com/HathorNetwork/hathor-wallet-headless/pull/237#issuecomment-1253730867

That, or maybe adding a slightly adapted form of that text to the readme.md file under a new Real-time Notifications section.

tuliomir

The code on this repository is approved by me.

The only thing I think is still good to have is to a complete sample of working code so that a developer can see a concrete example of how to put the plugin system to work. I just don't really see where to put this into this repository.

Maybe a tutorial text, a "Getting Started" repository ( containing integration tests ) on GitHub or a video on YouTube would be enough to give a bootstrap to a newcomer.

PLUGIN.md

config.js.template

pedroferreira1 · 2022-10-07T11:45:53Z

config.js.docker

@@ -74,6 +74,10 @@ const config = {
  connectionTimeout: getIntParam('connection_timeout'),
  allowPassphrase: getBooleanParam('allow_passphrase'),
  confirmFirstAddress: getBooleanParam('confirm_first_address'),
+
+  // Plugin configuration
+  enabled_plugins: getParam('enabled_plugins', '').split(' ').filter(x => x),


Why do you need this filter?

For the default value, since its an empty string.
After the split we will have [''] which was confusing, so I added this filter.

Maybe add a comment about this?

src/plugins/child.js

src/plugins/hathor_debug.js

src/plugins/hathor_sqs.js

src/services/notification.service.js

lgtm-com · 2022-10-11T20:14:12Z

This pull request introduces 1 alert when merging 44feb33 into e5de086 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

lgtm-com · 2022-10-11T20:46:19Z

This pull request introduces 1 alert when merging d83609a into e5de086 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

lgtm-com · 2022-10-13T18:31:24Z

This pull request introduces 1 alert when merging 967ac98 into e5de086 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

pedroferreira1 · 2022-10-14T17:42:54Z

src/helpers/notification.helper.js

+ *
+ * @returns {TokenBalance}
+ */
+function getWalletBalanceForTx(wallet, tx) {


I'm starting to think if this method should be in the lib. It seems a bit more complete than the one we have in the wallet class getTxBalance.

tuliomir

Code approved, and I left some suggestions for future refactors on the lib.

tuliomir · 2022-10-18T15:39:36Z

src/helpers/notification.helper.js

+        tokenBalance[token] = getEmptyBalance();
+      }
+
+      if ((input.token_data & TOKEN_AUTHORITY_MASK) > 0) {


I feel this could be added to the lib later as a isAuthorityInput(input) method, analog to the existing isAuthorityOutput(output), to maintain pattern of helper methods on the lib. The isMintInput and isMeltInput are good candidates too.

https://github.com/HathorNetwork/hathor-wallet-lib/blob/master/src/wallet.js#L1972-L1984

Well, the concept of a mint/melt input is somewhat strange to me.
The more correct idea is that the input spends a mint/melt output.
Even the token_data I used is because the fullnode adds some metadata of the utxo being spent in the input.

tuliomir · 2022-10-18T15:50:10Z

src/helpers/notification.helper.js

+    if (output.decoded && output.decoded.address && wallet.isAddressMine(output.decoded.address)) {
+      const { token } = output;
+      if (!tokenBalance[token]) {
+        tokenBalance[token] = getEmptyBalance();


Is it possible to reach this line?
If all tokens were scanned on the inputs, all of them must necessarily be present on the outputs too, right?

I'm just bringing this up because I can't see a way to make a test that covers this line and codecov will see this as a warning, but I think it may be good to keep the code this way.

Well, a token creation transaction does not have the created token on the inputs.

* dev: feat: external notifications (#237) chore: atomic swap api docs (#239)

r4mmer self-assigned this Sep 7, 2022

r4mmer marked this pull request as ready for review September 21, 2022 13:43

r4mmer requested a review from pedroferreira1 as a code owner September 21, 2022 13:43

r4mmer requested review from tuliomir and luislhl and removed request for luislhl September 21, 2022 13:44

tuliomir requested changes Sep 27, 2022

View reviewed changes

r4mmer added 5 commits September 27, 2022 14:48

feat: poc plugin system

1165dbb

feat: plugin system

36ef99a

chore: reset package-lock

fc557bb

feat: runtime dependency errors

5f8c417

feat: plugin documentation

ad9f959

r4mmer force-pushed the feat/external-notifications branch from 50dd3a3 to ad9f959 Compare September 30, 2022 00:05

r4mmer added 3 commits September 29, 2022 21:18

chore: linter changes

e18ee0f

chore: review changes

3cfc450

chore: rename event bus

f72b80c

r4mmer requested a review from tuliomir September 30, 2022 14:18

tuliomir approved these changes Oct 5, 2022

View reviewed changes

feat: add wallet balance to tx events

b2c442e

pedroferreira1 reviewed Oct 7, 2022

View reviewed changes

chore: review changes

44feb33

chore: change event names

d83609a

r4mmer added 2 commits October 13, 2022 11:45

chore: lint and unused test

e84235a

tests: notification helper

967ac98

chore: remove unused variable

f0fd0ca

r4mmer requested review from tuliomir and pedroferreira1 October 14, 2022 14:34

pedroferreira1 approved these changes Oct 14, 2022

View reviewed changes

chore: change name based on origin of event

31b646b

tuliomir approved these changes Oct 18, 2022

View reviewed changes

r4mmer mentioned this pull request Oct 19, 2022

Map wallet and node events with an enum HathorNetwork/hathor-wallet-lib#425

Open

r4mmer merged commit 6f0700b into dev Oct 19, 2022

r4mmer deleted the feat/external-notifications branch October 19, 2022 16:44

r4mmer added a commit that referenced this pull request Nov 1, 2022

Merge branch 'dev' into feat/readonly-wallets

c0fce91

* dev: feat: external notifications (#237) chore: atomic swap api docs (#239)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: external notifications #237

feat: external notifications #237

r4mmer commented Sep 7, 2022 •

edited

codecov bot commented Sep 7, 2022 •

edited

r4mmer commented Sep 21, 2022

tuliomir Sep 26, 2022

r4mmer Sep 30, 2022

tuliomir Oct 5, 2022

r4mmer Oct 19, 2022

tuliomir Sep 27, 2022

tuliomir Sep 27, 2022

tuliomir left a comment

pedroferreira1 Oct 7, 2022

r4mmer Oct 11, 2022

pedroferreira1 Oct 14, 2022

lgtm-com bot commented Oct 11, 2022

lgtm-com bot commented Oct 11, 2022

lgtm-com bot commented Oct 13, 2022

pedroferreira1 Oct 14, 2022

tuliomir left a comment

tuliomir Oct 18, 2022

r4mmer Oct 19, 2022

tuliomir Oct 18, 2022

r4mmer Oct 19, 2022

	// XXX: subscribe to wallet events with notificationBus
	notificationBus.subscribeHathorWallet(req.body['wallet-id'], wallet);

feat: external notifications #237

feat: external notifications #237

Conversation

r4mmer commented Sep 7, 2022 • edited

Acceptance Criteria

Description

Security Checklist

codecov bot commented Sep 7, 2022 • edited

Codecov Report

r4mmer commented Sep 21, 2022

FYI for reviewers

Running the plugins locally

debug plugin

websocket plugin

sqs

rabbitMQ plugin

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 left a 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

lgtm-com bot commented Oct 11, 2022

lgtm-com bot commented Oct 11, 2022

lgtm-com bot commented Oct 13, 2022

Choose a reason for hiding this comment

tuliomir left a 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

r4mmer commented Sep 7, 2022 •

edited

codecov bot commented Sep 7, 2022 •

edited