Review #242
Review #242
Conversation
Feat/deposit events leveldb
…efactoring Feat/deposit events verify refactoring
…efactoring Feat/deposit events verify refactoring
fix: migrate to confluentinc in kafka tests
…into feat/deposit-events-verify
feat: add KEYS_API_URL configuration
fix: invalid argument 1: hex number
Feat/signing keys registry
…rithm feat: new deposit registry algorithm
Get unvetted keys status from keys api
…ch-impr Review improvements
| blockHash: string; | ||
| blockNumber: number; |
There was a problem hiding this comment.
Since this object is used multiple times, we can create a type for it
| return ignoreDeposits; | ||
| } | ||
|
|
||
| public isNeedToProcessNewState(newMeta: { |
There was a problem hiding this comment.
shouldProcessNewState would be a better name
| const lastMeta = this.lastProcessedStateMeta; | ||
| if (!lastMeta) return true; | ||
| if (lastMeta.blockNumber > newMeta.blockNumber) { | ||
| this.logger.error('Keys API returns old state', { newMeta, lastMeta }); | ||
| return false; | ||
| } | ||
| const isSameBlock = lastMeta.blockHash === newMeta.blockHash; | ||
|
|
||
| if (isSameBlock) { | ||
| this.logger.log(`The block has not changed since the last cycle. Exit`, { | ||
| newMeta, | ||
| }); | ||
| } | ||
|
|
||
| return !isSameBlock; |
There was a problem hiding this comment.
It's kinda hard to reason with this logic because exits are not explicit. We can make it more clearer by rewriting it this way
| const lastMeta = this.lastProcessedStateMeta; | |
| if (!lastMeta) return true; | |
| if (lastMeta.blockNumber > newMeta.blockNumber) { | |
| this.logger.error('Keys API returns old state', { newMeta, lastMeta }); | |
| return false; | |
| } | |
| const isSameBlock = lastMeta.blockHash === newMeta.blockHash; | |
| if (isSameBlock) { | |
| this.logger.log(`The block has not changed since the last cycle. Exit`, { | |
| newMeta, | |
| }); | |
| } | |
| return !isSameBlock; | |
| if (!this.lastProcessedStateMeta) { | |
| return true; | |
| } | |
| if (newMeta.blockNumber < lastMeta.blockNumber) { | |
| this.logger.error('Keys API returns old state', { newMeta, lastMeta }); | |
| return false; | |
| } | |
| if (newMeta.blockHash === lastMeta.blockHash) { | |
| this.logger.log('The block has not changed since the last cycle. Exit', { newMeta }); | |
| return false; | |
| } | |
| return true; |
| const keyMap = keys.reduce((acc, key) => { | ||
| const duplicateKeys = acc.get(key.key) || []; | ||
| duplicateKeys.push(key); | ||
| acc.set(key.key, duplicateKeys); | ||
|
|
||
| return acc; | ||
| }, new Map<string, RegistryKey[]>()); | ||
|
|
||
| return Array.from(keyMap.entries()).filter( | ||
| ([, duplicateKeys]) => duplicateKeys.length > 1, | ||
| ); |
There was a problem hiding this comment.
We can find a lot of small optimizations all over this project by reducing the number of passes over an array of keys using for-loop instead of functional methods like reduce, filter, etc. I don't know if these optimizations are worth refactoring but are still worth pointing out.
For example, in this particular case we can do this one for-of loop instead doing a full reduce, from and filter on n keys.
e.g.
const keyMap = new Map<string, RegistryKey[]>();
const duplicateGroups: [string, RegistryKey[]][] = [];
for (const key of keys) {
const existingGroup = keyMap.get(key.key);
if (existingGroup) {
existingGroup.push(key);
if (existingGroup.length === 2) {
duplicateGroups.push([key.key, existingGroup]);
}
} else {
keyMap.set(key.key, [key]);
}
}
return duplicateGroups;There was a problem hiding this comment.
thank you for suggestion, but implicit data changes are not safe, and the small performance gains from this approach don't compensate for the risks. Safety and readability are far more important.
| return operatorKeys.reduce( | ||
| (prev, curr) => (prev.index < curr.index ? prev : curr), | ||
| operatorKeys[0], | ||
| ); |
There was a problem hiding this comment.
A little more readable way
| return operatorKeys.reduce( | |
| (prev, curr) => (prev.index < curr.index ? prev : curr), | |
| operatorKeys[0], | |
| ); | |
| return operatorKeys.sort((a, b) => a.index - b.index)[0]; |
| return this.handleDepositedKeyDuplicates(suspectedDuplicateKeys); | ||
| } | ||
|
|
||
| return await this.handleMultiOperatorDuplicates( |
There was a problem hiding this comment.
| return await this.handleMultiOperatorDuplicates( | |
| return this.handleMultiOperatorDuplicates( |
| duplicates, | ||
| ); | ||
|
|
||
| // Filter all unresolved keys (keys without a SigningKeyAdded event) for the module, |
| if (cacheResult === false) { | ||
| acc.cachedInvalidKeyList.push(key); | ||
| } | ||
|
|
||
| if (cacheResult === undefined) { | ||
| acc.uncachedDepositKeyList.push(depositKey); | ||
| } |
There was a problem hiding this comment.
This cache structure can potentially cause bugs because it can store two falsey values that mean completely different things: false means that the deposit data is invalid, but undefined means that it is not cached.
If you are not careful enough, you may accidentally check like so if (cacheResult), and it will lead to unexpected behavior.
I suggest that it's better to use explicit truthy values if the data is cached like, e.g.
const DEPOSIT_VALID = "valid";
const DEPOSIT_INVALID = "invalid";
cache.set(deposit, DEPOSIT_VALID);| this.securityService | ||
| .unvetSigningKeys( | ||
| nonce, | ||
| blockNumber, | ||
| blockHash, | ||
| stakingModuleId, | ||
| operatorIds, | ||
| vettedKeysByOperator, | ||
| signature, | ||
| ) | ||
| .catch((error) => | ||
| this.logger.error('Failed to send unvet transaction', { | ||
| error, | ||
| blockHash, | ||
| stakingModuleId, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
So, all guardians are potentially sending the same tx at the same time?
There was a problem hiding this comment.
It will be reverted for other councils if one has already been successfully sent
fix: update kapi version
PR for review DSM 1.5 and other changes