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]: Add Chain ID verification against RPC #37

Merged
merged 8 commits into from
Jul 2, 2024

Conversation

ajimeno04
Copy link
Contributor

This pull request introduces a new method to validate the Chain ID obtained from a given RPC endpoint against an expected Chain ID. The aim is to ensure that the application is connected to the correct blockchain network.

@ajimeno04 ajimeno04 requested a review from jsanmigimeno June 27, 2024 12:20
@ajimeno04 ajimeno04 linked an issue Jun 27, 2024 that may be closed by this pull request
@jsanmigimeno
Copy link
Member

This code is not correct, there are 2 main issues:

  • The result of the validateChainIdFromRPC() function is used in an incorrect way: if it returns true (i.e. the chainId is valid) an error is thrown.
  • validateChainIdFromRPC() is an async function, yet you are not awaiting the result. As a matter of fact, you cannot await the result where the function is called, since the loadChainsConfig() function is not an async one. You're going to have to explore at which other point you can make the async call, or how you can modify the current code to accommodate for it.

Copy link
Member

@jsanmigimeno jsanmigimeno left a comment

Choose a reason for hiding this comment

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

The current solution can result in inconsistent execution of the Relayer on startup, as you've wrapped the setting of the chainConfig within a promise on the loadChainsConfig() function. Since loadChainsConfig() is not an asynchronous function, and you're not awaiting the new logic, there is no guarantee that by the time loadChainsConfig() returns the config will be loaded. As a matter of fact, the configuration will not be set (you can verify this by setting a breakpoint after the call to loadChainsConfig() and inspecting the function output). The code does work, but just because the rest of the Relayer services take just enough time for the promise to resolve, but there is no guarantee that all the launched services will have had accessed the correct configuration. Such code creates bugs that are very hard to debug and should be avoided at all costs.

@ajimeno04 ajimeno04 requested a review from jsanmigimeno July 1, 2024 10:01
Copy link
Member

@jsanmigimeno jsanmigimeno left a comment

Choose a reason for hiding this comment

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

The code now works and is close to be ready. Requested changes are to include code quality improvements, and to make sure that the result of the newly introduced initialize() function is awaited (with the current version of the code this is strictly not required, but it can be confusing in the future that changes made within the initialize() function are not final before the Relayer intiates).

@@ -24,6 +25,11 @@ export class ConfigService {
this.globalConfig = this.loadGlobalConfig();
this.chainsConfig = this.loadChainsConfig();
this.ambsConfig = this.loadAMBsConfig();
this.initialize();
Copy link
Member

Choose a reason for hiding this comment

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

Save the promise to a this.isReady readonly variable and await that promise before the app initialization (i.e. before the line await app.listen(...) on main.ts)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check if I got the right idea on this comment

Copy link
Member

Choose a reason for hiding this comment

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

You are missing awaiting the isReady promise on main.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check if now is what you are saying. I think i got it but I'm no 100% sure

src/config/config.service.ts Outdated Show resolved Hide resolved
src/config/config.service.ts Outdated Show resolved Hide resolved
src/config/config.service.ts Outdated Show resolved Hide resolved
@ajimeno04 ajimeno04 requested a review from jsanmigimeno July 1, 2024 15:13
@jsanmigimeno jsanmigimeno force-pushed the Check_if_the_RPC_url_matches_the_ChainID branch from d6ae723 to fba4ada Compare July 2, 2024 09:46
@jsanmigimeno jsanmigimeno merged commit 587d234 into testnet Jul 2, 2024
1 check passed
@jsanmigimeno jsanmigimeno deleted the Check_if_the_RPC_url_matches_the_ChainID branch July 25, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feat]: Check if the RPC url matches the ChainID
2 participants