feat: reorgs outside of confirmations range handling #273

nduchak · 2020-08-21T13:45:56Z

No description provided.

lgtm-com · 2020-08-21T13:49:50Z

This pull request introduces 2 alerts when merging 55be7e6 into a5b3a18 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

refactor db-backup job

lgtm-com · 2020-08-21T15:16:12Z

This pull request introduces 1 alert when merging e1fcb73 into a5b3a18 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

lgtm-com · 2020-08-21T15:23:36Z

This pull request introduces 1 alert when merging 626cec7 into a5b3a18 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

add basic unit test for backup job constructor

lgtm-com · 2020-08-24T11:07:20Z

This pull request introduces 1 alert when merging 0699b59 into a5b3a18 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

lgtm-com · 2020-08-24T13:29:23Z

This pull request introduces 1 alert when merging 1dd69b7 into a5b3a18 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

lgtm-com · 2020-08-24T13:37:08Z

This pull request introduces 3 alerts when merging ca21a7c into a5b3a18 - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

lgtm-com · 2020-08-24T15:23:22Z

This pull request introduces 2 alerts when merging 0a88d6a into a5b3a18 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

lgtm-com · 2020-08-24T15:32:01Z

This pull request introduces 2 alerts when merging 466b556 into a5b3a18 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

lgtm-com · 2020-08-25T11:47:08Z

This pull request introduces 2 alerts when merging df64345 into a5b3a18 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

lgtm-com · 2020-08-25T12:04:19Z

This pull request introduces 2 alerts when merging 127dc78 into a5b3a18 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

add reorg service to blockchain init and channels subscribe for reorgs for each of services and emit using reorg service

lgtm-com · 2020-08-26T11:05:47Z

This pull request introduces 2 alerts when merging 754b2bc into a5b3a18 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

lgtm-com · 2020-08-26T15:21:42Z

This pull request introduces 2 alerts when merging b6badf2 into a5b3a18 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

lgtm-com · 2020-08-26T16:52:29Z

This pull request introduces 2 alerts when merging 2cfd958 into a5b3a18 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

lgtm-com · 2020-08-27T08:11:05Z

This pull request introduces 2 alerts when merging efca52b into a5b3a18 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

lgtm-com · 2020-08-28T15:04:52Z

This pull request introduces 2 alerts when merging 7b76fd4 into 25db5ac - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

lgtm-com · 2020-08-28T15:10:45Z

This pull request introduces 2 alerts when merging e17acb1 into 25db5ac - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

AuHau

Hmm there are few questions and some things to improve. But it is on good path!

src/cli/start.ts

AuHau · 2020-09-02T08:11:08Z

src/cli/start.ts


-    const app = await appFactory()
-    const port = config.get('port')
-    const server = app.listen(port)


To be honest, I don't really like moving the server start&listening logic to the appFactory(). I don't actually think that it has to be there right? What was your motivation here?

In this shape it is not a "factory" anymore...

Just want to make the same as we had it in pinner, as we use the same algorithm for reorgs

Hmmmmmm. I see you renamed it to startApp. I am still against this refactor. While I understand your motivation, the Pinner and Cache are different things, using different setups (Cache uses Feathers and it is a Web Server, Pinner is a specific custom-built daemon service). Yes, we use the same mechanism for handling reorgs, but the context is a bit different. You can still use this "algorithm" even with the original approach of appFactory()...

I think that bootstrapping the Feather's app and actually starting the listening on a port should be kept separate.

Reverted back

src/services/storage/index.ts

src/utils.ts

src/blockchain/reorg-emitter.ts

src/utils.ts

AuHau · 2020-09-03T08:25:38Z

src/blockchain/utils.ts

@@ -29,7 +28,7 @@ export function isServiceInitialized (serviceName: string): boolean {
  return blockTracker.getLastFetchedBlock()[0] !== undefined
 }

-export function getEventsEmitterForService (serviceName: string, eth: Eth, contractAbi: AbiItem[]): EventEmitter {
+export function getEventsEmitterForService (serviceName: string, eth: Eth, contractAbi: AbiItem[]): BaseEventsEmitter {


Why do you need the BaseEvemtsEmitter?

I need the stop method from BaseEvemtsEmitter

AuHau · 2020-09-03T08:29:08Z

src/definitions.ts

@@ -28,7 +29,8 @@ export enum ServiceAddresses {
  STORAGE_AGREEMENTS = '/storage/v0/agreements',
  XR = '/rates/v0/',
  CONFIRMATIONS = '/confirmations',
-  NEW_BLOCK_EMITTER = '/new-block'
+  NEW_BLOCK_EMITTER = '/new-block',
+  REORG_EMITTER = '/reorg'


Hmm I think we should incorporate in the naming of variables, paths etc the fact the this is not normal reorg but reorg outside of confirmation range. Because these two are different and it can lead to confusion later on.

I think it's fine as we can have different events based on this path. Separation happens there

AuHau · 2020-09-03T08:48:49Z

src/utils.ts

+type BackUpEntry = { name: string, block: { hash: string, number: BigNumber } }
+
+function parseBackUps (backUpName: string): BackUpEntry {
+  const [block] = backUpName.split('.')[0].split('-')


Hmm what length of the names you usually get? Just to make sure we won't run in problems with FS...
My original idea was actually store these data as part of the Store, but I guess this will work as well 😊

Not too big, like: add-name-to-user

AuHau · 2020-09-03T09:28:43Z

src/cli/start.ts

+      backUpJob.stop()
+
+      // Restore DB from backup
+      await backUpJob.restoreDb(() => undefined)


Why do you "ignore" the errorCallback here? And actually why there is even such a thing? Isn't rejected Promise enough?

AuHau

Hmm still some things to tackle.

Btw. did you test how the UI handles when Cache disconnects? We should know its behavior as maybe there should be some added logic to the UI for handling when Cache is not online and also to listen on the Reorg channel...

Also, please don' do force-push once the review process starts. It makes it hard for me to track the increment changes you are making especially in big PRs like this...

AuHau · 2020-09-07T08:34:38Z

src/cli/start.ts


-    const app = await appFactory()
-    const port = config.get('port')
-    const server = app.listen(port)


Hmmmmmm. I see you renamed it to startApp. I am still against this refactor. While I understand your motivation, the Pinner and Cache are different things, using different setups (Cache uses Feathers and it is a Web Server, Pinner is a specific custom-built daemon service). Yes, we use the same mechanism for handling reorgs, but the context is a bit different. You can still use this "algorithm" even with the original approach of appFactory()...

I think that bootstrapping the Feather's app and actually starting the listening on a port should be kept separate.

src/db-backup.ts

config/default.json5

test/cli.spec.ts

test/db-backup-restore.spec.ts

AuHau · 2020-09-07T09:10:53Z

src/cli/start.ts

+      backUpJob.stop()
+
+      // Restore DB from backup
+      await backUpJob.restoreDb()


Hmmm so if the restore procedure fails for some reason then Cache craches, right? 🤔

Yes, it's true. For that case, we should notify DevOps that the restore process fails

src/cli/start.ts

sonarqubecloud · 2020-09-07T11:36:23Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
6 Code Smells

No Coverage information
10.9% Duplication

src/db-backup.ts

AuHau

LGTM! Thanks!

chore: draft for backup/restore of db

55be7e6

chore: add stop method for rate service

e1fcb73

refactor db-backup job

nduchak added 2 commits August 21, 2020 18:19

chore: use async fs methods for copy/rename/delete in db restore/backup

626cec7

chore: fix linter

cb236cf

nduchak added 2 commits August 24, 2020 11:41

feat: use bignumber for db backup

4ff6cfd

feat: fix get emitters config method

0699b59

add basic unit test for backup job constructor

feat: finish backup tests

ca21a7c

nduchak force-pushed the feat/reorgs branch from 1dd69b7 to ca21a7c Compare August 24, 2020 13:29

feat: add restore db tests

0a88d6a

chore: fix lint

466b556

chore: small adjustment

df64345

chore: fix some code smells

127dc78

feat: add basic reorg emitter service

754b2bc

add reorg service to blockchain init and channels subscribe for reorgs for each of services and emit using reorg service

feat: add timeout for emitting reorgs

b6badf2

feat: add reorg handler to app.js

efca52b

nduchak force-pushed the feat/reorgs branch from 2cfd958 to efca52b Compare August 27, 2020 08:05

nduchak force-pushed the feat/reorgs branch from 8b37843 to 7b76fd4 Compare August 28, 2020 15:01

chore: fix linter

3133aa0

chore: add config mutation env var to circleci

e17acb1

chore: fix index file

5adcd90

nduchak requested a review from AuHau August 31, 2020 11:37

nduchak self-assigned this Aug 31, 2020

nduchak added the P1 - High label Aug 31, 2020

nduchak added this to the v0.1.0 - First release milestone Aug 31, 2020

nduchak added 2 commits August 31, 2020 14:44

chore: increase timeout for CLI test

fea1cd8

chore: add db back-up to default config

3e7e4e9

nduchak marked this pull request as ready for review September 1, 2020 14:42

chore: polish configs

eb11972

AuHau suggested changes Sep 3, 2020

View reviewed changes

nduchak force-pushed the feat/reorgs branch 2 times, most recently from c6fb36d to 9836164 Compare September 4, 2020 09:24

chore: fix pr comments

05d6918

nduchak force-pushed the feat/reorgs branch from 9836164 to 05d6918 Compare September 4, 2020 09:27

chore: fix tests

8ff476f

AuHau suggested changes Sep 7, 2020

View reviewed changes

feat: fix most of pr comments

13e1a7c

AuHau reviewed Sep 7, 2020

View reviewed changes

src/db-backup.ts Show resolved Hide resolved

nduchak mentioned this pull request Sep 7, 2020

Add ability to send notification to DevOps #288

Open

AuHau approved these changes Sep 8, 2020

View reviewed changes

AuHau changed the title ~~feat: reorgs~~ feat: reorgs outside of confirmations range handling Sep 8, 2020

AuHau merged commit 4d48f80 into master Sep 8, 2020

AuHau deleted the feat/reorgs branch September 8, 2020 07:16

AuHau mentioned this pull request Sep 15, 2020

Handle reorg outside of confirmation range #192

Closed

feat: reorgs outside of confirmations range handling #273

feat: reorgs outside of confirmations range handling #273

Conversation

nduchak commented Aug 21, 2020

lgtm-com bot commented Aug 21, 2020

lgtm-com bot commented Aug 21, 2020

lgtm-com bot commented Aug 21, 2020

lgtm-com bot commented Aug 24, 2020

lgtm-com bot commented Aug 24, 2020

lgtm-com bot commented Aug 24, 2020

lgtm-com bot commented Aug 24, 2020

lgtm-com bot commented Aug 24, 2020

lgtm-com bot commented Aug 25, 2020

lgtm-com bot commented Aug 25, 2020

lgtm-com bot commented Aug 26, 2020

lgtm-com bot commented Aug 26, 2020

lgtm-com bot commented Aug 26, 2020

lgtm-com bot commented Aug 27, 2020

lgtm-com bot commented Aug 28, 2020

lgtm-com bot commented Aug 28, 2020

AuHau 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

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

AuHau left a comment • 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

sonarqubecloud bot commented Sep 7, 2020

AuHau left a comment

Choose a reason for hiding this comment

AuHau left a comment •

edited

Loading