Skip to content
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

feat: call onNewNftEvent when a new NFT tx is received #150

Merged
merged 24 commits into from
Apr 19, 2024

Conversation

andreabadesso
Copy link
Collaborator

@andreabadesso andreabadesso commented Mar 28, 2024

Motivation

This fixes #153

After the wallet-service refactor, we're no longer calling the old onNewTxRequest lambda (as what it used to do is now done by the daemon), so the logic that deals with NFTs should be called from the daemon.

This PR is not attempting to refactor the wallet-service and move all shared code to the common package, but to move only the minimum necessary code so that the nft util works both on the wallet-service and the daemon package.

Acceptance Criteria

  • We should move the nft utils from the wallet-service repository to the common package, that has methods common to both the lambdas and the daemon
  • We should move all transaction types to the common package
  • We should call use the nft util methods from the common package to call the onNewNftEvent lambda in the newVertexAccepted service of 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 andreabadesso self-assigned this Mar 28, 2024
@andreabadesso andreabadesso added the enhancement New feature or request label Mar 28, 2024
package.json Outdated
],
"engines": {
"node": ">=18"
},
"nohoist": [
"**"
],
"repository": "[email protected]:HathorNetwork/hathor-wallet-service-sync_daemon.git",
"repository": "[email protected]:HathorNetwork/hathor-wallet-service.git",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I renamed the repository

Comment on lines 5 to 10
"@aws-sdk/client-lambda": "3.540.0",
"@hathor/wallet-lib": "0.39.0",
"winston": "^3.13.0"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are defined as peerDependencies so they use the same version from the root package.json

@andreabadesso andreabadesso force-pushed the feat/send-nft branch 2 times, most recently from b75dff9 to 438e9c9 Compare April 1, 2024 00:05
@andreabadesso andreabadesso changed the base branch from master to refactor/common-project April 1, 2024 00:05
@@ -75,6 +75,7 @@ export type FullNodeEvent = {
tokens: string[];
token_name: null | string;
token_symbol: null | string;
signal_bits: number;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should open a PR to start storing this in the transactions table, but this is not yet coming from the event stream.

@andreabadesso andreabadesso force-pushed the feat/send-nft branch 4 times, most recently from a6ee5ce to 10a9f49 Compare April 1, 2024 17:52
}

// TODO: Send message on SQS for real-time update
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We already did this in the past, it's on line 327

@andreabadesso andreabadesso force-pushed the feat/send-nft branch 2 times, most recently from cf8b820 to 0ebc4d8 Compare April 1, 2024 18:19
@andreabadesso andreabadesso changed the title feat: send nft metadata feat: call onNewNftEvent when a new NFT tx is received Apr 1, 2024
Copy link

Choose a reason for hiding this comment

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

We should have the Hathor header on this file.

Also, this is a types file that is half filled with utils functions. Maybe this is not the time to extract all those functions into separate dedicated files, but I believe at least an issue would be important to do so. In the future we may have circular dependencies because of these mixed responsibilities.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adde the header in 61fe30e

I'm writing a project listing all refactors I think we need to do to share code between projects, I'll add this there

@andreabadesso andreabadesso force-pushed the refactor/common-project branch 3 times, most recently from deb36e6 to 8da7285 Compare April 3, 2024 15:22
@andreabadesso andreabadesso force-pushed the feat/send-nft branch 2 times, most recently from 1f867a2 to cdf68cc Compare April 3, 2024 16:12
@andreabadesso andreabadesso force-pushed the refactor/common-project branch from fe72085 to e4200a0 Compare April 5, 2024 15:25
@andreabadesso andreabadesso force-pushed the feat/send-nft branch 2 times, most recently from b8783e9 to dfb1b61 Compare April 5, 2024 16:26
@andreabadesso andreabadesso force-pushed the feat/send-nft branch 2 times, most recently from c3c1ca1 to 0de6578 Compare April 19, 2024 19:45
* tests: fixed nft tests on txProcessor

* refactor: removed all methods related to the old wallet-service txProcessor
@andreabadesso andreabadesso merged commit c19c2c3 into refactor/common-project Apr 19, 2024
1 check passed
@andreabadesso andreabadesso deleted the feat/send-nft branch April 19, 2024 22:30
andreabadesso added a commit that referenced this pull request Apr 26, 2024
* 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
andreabadesso added a commit that referenced this pull request Apr 30, 2024
* 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
andreabadesso added a commit that referenced this pull request May 1, 2024
* 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
andreabadesso added a commit that referenced this pull request May 4, 2024
* 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
andreabadesso added a commit that referenced this pull request May 30, 2024
* chore: added common package

* chore: using wallet-lib from daemon resolution

* chore: installed shared dependencies on root project, using peerDependencies

* refactor: using addAlert method from common utils

* chore: updated package.json with missing deps

* refactor: using types from commons on wallet-service

* chore: re-added sequelize to root

* refactor: removed isNftAutoReviewEnabled from services

* tests: mocked assertEnv

* tests: mocked assertEnv on integration tests

* chore: removed nft utils

* refactor: removed old addAlerts from the wallet-service package

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

* refactor: logger is now a required param in addAlert, refactored all methods in wallet-service package

* docs: added missing hathor header

* refactor: invalid type for metadata on addAlert

* tests: passing mocked logger

* chore: updated gitignore to ignore env files

* chore: revert eslint package updates

* refactor: using types from commons on wallet-service

* refactor: removed unused commented line

* chore: missing package in daemon

* refactor: using transaction type from common

* refactor: bitcoinjs is not a shared dep

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

* refactor: removed default from metadata

* chore: lint fixes and package ordering

* feat: call onNewNftEvent when a new NFT tx is received (#150)

* 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

* tests: fixed failing test

* fix: serverless failing (#162)

* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants