client/sync: return Promise.race() instead of async-promise-executor

[ScottyPoi]

similar to #3015. Rewrites sync() without the async-promise-generator api.

--

The Promise returned at the end of the original code was designed to resolve either when a SYNC_SYNCHRONIZED event is emitted or when await _fetcher.fetch() return/throws.

This was done using an async-promise-executor:
return new Promise( async (res, rej) => { ... } )

This is generally considered an anti-pattern, and violated our eslint rules, requiring an ignore comment.

--
Refactored by breaking sync() up into smaller methods: sync(), syncWithFetcher(), and resolveSync(), and returning Promise.race([...]) instead of the async promise executor.

1. resolveSync() externalized to this.resolveSync()
resolveSync() is called as a final step to clear the fetcher and emit a debug log. This was defined inside of the sync() function, but is now defined independently.

2. syncWithFetcher
   The internal try/catch block that contains await this._fetcher.fetch(). Now externalized to an external method this.syncWithFetcher().

3. syncEvent
   The SYNC_SYNCHRONIZED event that can resolve sync() is now defined as a Promise.

4. Return Promise
   sync() now returns Promise.race([this.syncWithFetcher(), syncEvent]). Resolving through whichever happens first.

[codecov]

Codecov Report

Merging #3030 (909b189) into master (d034e14) will increase coverage by 0.07%.
The diff coverage is 87.17%.

Additional details and impacted files

Flag	Coverage Δ
block	88.73% <ø> (ø)
blockchain	92.58% <ø> (ø)
client	87.69% <87.17%> (+0.13%)	⬆️
common	98.19% <ø> (ø)
ethash	?
evm	71.75% <ø> (ø)
rlp	∅ <ø> (?)
statemanager	90.15% <ø> (ø)
trie	90.34% <ø> (+0.27%)	⬆️
tx	96.35% <ø> (ø)
util	86.78% <ø> (ø)
vm	78.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[holgerd77]

Updated this via UI

[holgerd77]

Seems one client test might be failing here repeatedly, so this is eventually related to the change done and needs some investigation.

[ScottyPoi]

Seems one client test might be failing here repeatedly, so this is eventually related to the change done and needs some investigation.

The fullsync.spec.ts test was failing after the initial commits, even though all other sync related unit and integration tests were passing. Investigating led me to believe that this was indeed a flaw in the refactor.

I worked out a solution that fixed that test, but now I am getting errors from lightsync and beaconsync that I do not understand.

[ScottyPoi]

 FAIL  test/sync/beaconsync.spec.ts [ test/sync/beaconsync.spec.ts ]
 FAIL  test/sync/fetcher/reverseblockfetcher.spec.ts [ test/sync/fetcher/reverseblockfetcher.spec.ts ]
TypeError: Class extends value undefined is not a constructor or null
 ❯ src/sync/lightsync.ts:17:40
     15|  * @memberof module:sync
     16|  */
     17| export class LightSynchronizer extends Synchronizer {
       |                                        ^
     18|   constructor(options: SynchronizerOptions) {
     19|     super(options)
 ❯ src/service/lightethereumservice.ts:2:31
 ❯ src/service/index.ts:3:31

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/5]⎯

 FAIL  test/sync/lightsync.spec.ts [ test/sync/lightsync.spec.ts ]
 FAIL  test/sync/snapsync.spec.ts [ test/sync/snapsync.spec.ts ]
 FAIL  test/sync/sync.spec.ts [ test/sync/sync.spec.ts ]
TypeError: Class extends value undefined is not a constructor or null
 ❯ src/sync/beaconsync.ts:28:41
     26|  * @memberof module:sync
     27|  */
     28| export class BeaconSynchronizer extends Synchronizer {
       |                                         ^
     29|   skeleton: Skeleton
     30|   private execution: VMExecution
 ❯ src/sync/index.ts:1:31
 ❯ src/service/fullethereumservice.ts:11:3

[holgerd77]

@g11tech could you give this here a quick review? I think you are better on judging on the implications of this, I am always struggling with all-things-promises.

[holgerd77]

Updated this via UI

[acolytec3]

Great clean-up.

[g11tech]

lgtm great cleanup ❤️