refactor: added a "common" package to share code between packages #151

andreabadesso · 2024-04-01T00:02:42Z

Motivation

We should have a common package so that the wallet-service and the daemon can share code between them.

This PR does not attempt to refactor both projects, but only to create the common project and include the bare minimum changes to enable code sharing of the nft util

Acceptance Criteria

We should migrate the addAlert from the daemon's package to the common package and use it on both the wallet-service and the daemon
We should migrate types related to transactions and use them in the daemon

Checklist

If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged
Make sure either the unit tests and/or the QA tests are capable of testing the new features
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.

andreabadesso · 2024-04-01T17:41:47Z

package.json

+    "@aws-sdk/client-apigatewaymanagementapi": "3.540.0",
+    "@aws-sdk/client-lambda": "3.540.0",
+    "@aws-sdk/client-sqs": "3.540.0",
+    "@hathor/wallet-lib": "0.39.0",
+    "bip32": "^4.0.0",
+    "bitcoinjs-lib": "^6.1.5",
+    "bitcoinjs-message": "^2.2.0",
+    "tiny-secp256k1": "^2.2.3",
+    "winston": "^3.13.0"


Added those dependencies here so the projects can use peerDependencies and share the dependency

packages/common/src/utils/index.utils.ts

pedroferreira1 · 2024-04-04T23:08:56Z

packages/common/src/utils/alerting.utils.ts

@@ -28,9 +28,9 @@ export const addAlert = async (
  title: string,
  message: string,
  severity: Severity,
-  metadata?: unknown,
+  metadata: unknown = null,


Why did you set this default value? Shouldn't we have the optional parameters in the end of the method?

Added a comment in this line: a410a8c

I'll refactor the logger to be shared between projects, so logger will stop being a parameter there, so this default to null is temporary until then

I still don't understand the default null. We use default values for parameters in the end of the list of parameters, which is not the case.

Removed the null default in cae81ac

packages/daemon/package.json

packages/wallet-service/package.json

pedroferreira1 · 2024-04-04T23:21:31Z

packages/wallet-service/src/types.ts

-  WARNING = 'warning',
-  INFO = 'info',
-}
+export { Severity, Transaction, TxInput, TxOutput, DecodedOutput };


Why do you need to export the imported types?

We don't, this was done initially because I was only planning to move the types on the next PR

But refactored the code to import from the common project, can you please check again?

package.json

tuliomir · 2024-04-09T15:43:37Z

packages/wallet-service/tests/integration.test.ts

@@ -4,7 +4,8 @@ import eventTemplate from '@events/eventTemplate.json';
 import { loadWallet, loadWalletFailed } from '@src/api/wallet';
 import { createWallet, getMinersList } from '@src/db';
 import * as txProcessor from '@src/txProcessor';
-import { Transaction, WalletStatus, TxInput, Severity } from '@src/types';
+import { WalletStatus  } from '@src/types';


style: Lint fix

Suggested change

import { WalletStatus } from '@src/types';

import { WalletStatus } from '@src/types';

Done in 408d995

tuliomir · 2024-04-09T15:44:41Z

packages/wallet-service/tests/utils/pushnotification.utils.test.ts

@@ -6,7 +6,8 @@ import { sendMulticastMock, messaging, initFirebaseAdminMock } from '@tests/util
 import { logger } from '@tests/winston.mock';
 import { PushNotificationUtils, PushNotificationError, buildFunctionName, FunctionName } from '@src/utils/pushnotification.utils';
 import * as pushnotificationUtils from '@src/utils/pushnotification.utils';
-import { SendNotificationToDevice, Severity } from '@src/types';
+import { SendNotificationToDevice } from '@src/types';
+import { Severity  } from '@wallet-service/common/src/types';


style: Lint fix

Suggested change

import { Severity } from '@wallet-service/common/src/types';

import { Severity } from '@wallet-service/common/src/types';

Done in 408d995

tuliomir · 2024-04-09T15:52:18Z

packages/common/package.json

+  "peerDependencies": {
+    "@aws-sdk/client-lambda": "3.540.0",
+    "@hathor/wallet-lib": "0.39.0",
+    "winston": "^3.13.0"


question(non-blocking): Here we have a mix of version definitions. Some are pinned to an exact value and some are with a flexible caret range.

Should we decide for one approach or another? Or can we leave this mixed as it currently is?

I'll fix all versions in a future PR

tuliomir · 2024-04-09T15:56:26Z

package.json

    "packages/daemon",
-    "packages/wallet-service"
+    "packages/wallet-service",
+    "packages/common"


suggestion: Order this alphabetically, as it will probably be automatically fixed at some point in the future

"packages/common", "packages/daemon", "packages/wallet-service"

Done in 408d995

…dencies

…methods in wallet-service package

* feat: added nft utils * tests: added tests for common utils * chore: added common module to CI * refactor: moved used types to common package * tests: removed nft utils * tests: fixed nft tests on txProcessor * tests: mocking network on getconfig * tests: fixed nft tests on txProcessor * refactor: passing logger to invoke nft handler * refactor: no need to ignore ts * chore: removed jest script, instead using only test * chore: added hathor header * refactor: using common utils on txProcessor * tests: nft utils using old syntax * tests: skipped txProcessor tests * tests: fixed txProcessor tests * refactor: using isAuthority from common utils * refactor: using isAuthority from common types * refactor: using assertEnvVariablesExistance from common project * refactor: getting CREATE_NFT_MAX_RETRIES from env * docs: updated docstrings on nft utils * chore: removed events/nftCreationTx.ts * refactor: invalid import locations * refactor: remove unused lambdas (#155) * tests: fixed nft tests on txProcessor * refactor: removed all methods related to the old wallet-service txProcessor

* refactor: logger is now a required param in addAlert, refactored all methods in wallet-service package * chore: changed module resolution and several package locks * chore: added common library to externals whitelist and whitelisted it in tsloader * chore: wallet-service common package moved from direct path to workspace * chore: added common as a peer dependency and restored peer dependencies * chore: removed peer dependencies as they were not working with serverless-monorepo * chore: added comment on hoisting limits * chore: added comment explaining externals in webpack

andreabadesso self-assigned this Apr 1, 2024

andreabadesso added the enhancement New feature or request label Apr 1, 2024

andreabadesso commented Apr 1, 2024

View reviewed changes

andreabadesso force-pushed the refactor/common-project branch from e66536d to 81d2a6d Compare April 1, 2024 18:10

andreabadesso requested review from tuliomir, r4mmer and pedroferreira1 and removed request for r4mmer April 1, 2024 19:21

andreabadesso changed the title ~~refactor: common project~~ refactor: added a "common" package to share code between packages Apr 1, 2024

tuliomir approved these changes Apr 2, 2024

View reviewed changes

packages/common/src/utils/index.utils.ts Outdated Show resolved Hide resolved

andreabadesso force-pushed the refactor/common-project branch 5 times, most recently from deb36e6 to 8da7285 Compare April 3, 2024 15:22

tuliomir approved these changes Apr 3, 2024

View reviewed changes

pedroferreira1 reviewed Apr 4, 2024

View reviewed changes

andreabadesso force-pushed the refactor/common-project branch 3 times, most recently from 1d73093 to 49a3744 Compare April 5, 2024 16:13

andreabadesso requested review from pedroferreira1 and tuliomir April 8, 2024 19:50

tuliomir approved these changes Apr 9, 2024

View reviewed changes

andreabadesso force-pushed the refactor/common-project branch from 6506f1f to 8604db6 Compare April 15, 2024 01:32

pedroferreira1 approved these changes Apr 18, 2024

View reviewed changes

andreabadesso force-pushed the refactor/common-project branch 2 times, most recently from 16ba61f to 9d84734 Compare May 1, 2024 17:57

andreabadesso added 3 commits May 3, 2024 21:52

chore: added common package

c316924

chore: using wallet-lib from daemon resolution

5a1d21d

chore: installed shared dependencies on root project, using peerDepen…

09a51d7

…dencies

andreabadesso added 23 commits May 3, 2024 21:52

refactor: using types from commons on wallet-service

23acb9e

chore: re-added sequelize to root

02ed3b7

refactor: removed isNftAutoReviewEnabled from services

18f7fe5

tests: mocked assertEnv

da614c6

tests: mocked assertEnv on integration tests

b49cf75

chore: removed nft utils

cd0c540

refactor: removed old addAlerts from the wallet-service package

1f00aa2

chore: wallet-lib is now a peerDependency in wallet-service package

2ff158c

refactor: logger is now a required param in addAlert, refactored all …

f48f3be

…methods in wallet-service package

docs: added missing hathor header

d74c702

refactor: invalid type for metadata on addAlert

c6ab0f7

tests: passing mocked logger

572aba9

chore: updated gitignore to ignore env files

3968f84

chore: revert eslint package updates

525f04b

refactor: using types from commons on wallet-service

e386ec6

refactor: removed unused commented line

9b3623c

chore: missing package in daemon

86ad635

refactor: using transaction type from common

4c4b08d

refactor: bitcoinjs is not a shared dep

8c22af7

chore: added a comment explaining that logger is a param temporarily

6af96eb

refactor: removed default from metadata

7fb0971

chore: lint fixes and package ordering

5881f32

andreabadesso force-pushed the refactor/common-project branch from 9d84734 to 25261a1 Compare May 4, 2024 00:53

andreabadesso added 2 commits May 29, 2024 18:35

tests: fixed failing test

856b5cc

andreabadesso merged commit 5f83d3e into master May 30, 2024
1 check passed

andreabadesso deleted the refactor/common-project branch May 30, 2024 00:58

This was referenced Jun 13, 2024

Release candidate v1.6.2 #175

Merged

Release v1.6.2 #176

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: added a "common" package to share code between packages #151

refactor: added a "common" package to share code between packages #151

andreabadesso commented Apr 1, 2024 •

edited

Loading

andreabadesso Apr 1, 2024

pedroferreira1 Apr 4, 2024

andreabadesso Apr 5, 2024 •

edited

Loading

pedroferreira1 Apr 8, 2024

andreabadesso Apr 15, 2024

pedroferreira1 Apr 4, 2024

andreabadesso Apr 5, 2024

tuliomir Apr 9, 2024

andreabadesso Apr 11, 2024 •

edited

Loading

tuliomir Apr 9, 2024

andreabadesso Apr 11, 2024 •

edited

Loading

tuliomir Apr 9, 2024

andreabadesso Apr 11, 2024

tuliomir Apr 9, 2024

andreabadesso Apr 11, 2024 •

edited

Loading

	import { WalletStatus } from '@src/types';
	import { WalletStatus } from '@src/types';

	import { Severity } from '@wallet-service/common/src/types';
	import { Severity } from '@wallet-service/common/src/types';

refactor: added a "common" package to share code between packages #151

refactor: added a "common" package to share code between packages #151

Conversation

andreabadesso commented Apr 1, 2024 • edited Loading

Motivation

Acceptance Criteria

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreabadesso Apr 5, 2024 • edited Loading

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

andreabadesso Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreabadesso Apr 11, 2024 • edited Loading

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

andreabadesso Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

andreabadesso commented Apr 1, 2024 •

edited

Loading

andreabadesso Apr 5, 2024 •

edited

Loading

andreabadesso Apr 11, 2024 •

edited

Loading

andreabadesso Apr 11, 2024 •

edited

Loading

andreabadesso Apr 11, 2024 •

edited

Loading