Add chain-based event filtering #1920
Conversation
hectorgomezv
changed the title
Pull Request Test Coverage Report for Build 10847874852Details
💛 - Coveralls |
src/domain/hooks/hooks.repository.ts
Outdated
| }); | ||
| } else { | ||
| this.loggingService.warn({ | ||
| type: 'unsupported_event_chain', |
There was a problem hiding this comment.
If this follows a naming convention we can leave it as is, but unsupported_chain_event reads better (also would need changing in the other service).
There was a problem hiding this comment.
Totally agree! Changed in 789cee5
src/domain/hooks/hooks.repository.ts
Outdated
| } | ||
| } | ||
|
|
||
| private async isSupportedChain(chainId: string): Promise<boolean> { |
There was a problem hiding this comment.
Nit: I think this should "live" in the chains repository and we then expose a clearSupportedChain method for consistency. What do you think?
There was a problem hiding this comment.
I didn't move the memoized function to the ChainsRepository because I think it could add more complexity (other classes might not need to call a memoized function I think). So I've left the clearing of the memoized function cache responsibility in the HooksRepository for now. If memoizing makes sense in other high-load places we could think on memoizing inside the ChainsRepository (or even on other repositories), but I tend to think it is out of the scope of the PR, wdyt?
Co-authored-by: Aaron Cook <aaron@safe.global>
Co-authored-by: Aaron Cook <aaron@safe.global>
Co-authored-by: Aaron Cook <aaron@safe.global>
| @@ -328,6 +348,10 @@ export class HooksRepositoryWithNotifications implements IHooksRepository { | |||
| ); | |||
| break; | |||
| case ConfigEventType.CHAIN_UPDATE: | |||
| // As the chains have been updated, we need to clear the memoized function cache. | |||
| if (this.isSupportedChainMemo.cache.clear) { | |||
There was a problem hiding this comment.
Suggestion: What do you think about creating a private method to handle this condition?
The current method is nearly 300 lines long, so splitting it could improve readability. It’s fine for now, but it seems like the entire method could benefit from being refactored.
There was a problem hiding this comment.
Absolutely! Good point, I think we could move all the code within each case to private methods to improve the overall readability. Are you OK if I do that on a separate PR?
There was a problem hiding this comment.
@hectorgomezv Absolutely, that's a good idea.
There was a problem hiding this comment.
I've created an issue to keep track of it 👍
Absolutely, I think this suggestion from @PooyaRaki #1920 (comment) would also help, as both repositories could reference the same private functions if I'm not wrong. |
Summary
This PR adds a check to the
onEventcallback ofHooksRepositoryWithNotifications/HooksRepositoryto check thechainIdin the event being processed is related to a knownChain. If it isn't, a warning log is emitted, instead of letting the service execute the event handler (and fail).Note: Ideally the unsupported events should be grouped to reduce the amount of potential logs. A forthcoming PR will take care of the grouping.
Changes
isSupportedChaintoIChainsRepository(and its memorization wrapperisSupportedChainMemotoHooksRepositoryWithNotifications/HooksRepository). This function checks the validity of the chain in the event being processed.