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

Reusable server module #1637

Open
wants to merge 15 commits into
base: staging
Choose a base branch
from

Conversation

RaoulSchaffranek
Copy link

Overview

This PR aims to make the sourcify-server module available as a library that can be distributed via package registries like npm.

Background

We are developing a Solidity debugger that utilizes Sourcify as a source-code repository. Our goal is to leverage Sourcify for debugging public transactions on mainnet and testnets, as well as on local testnets such as Anvil. Given the complexity of installing Sourcify, we plan to bundle a Sourcify server within our VSCode extension and manage the server process ourselves for local development.

Changes Summary

To achieve this, the following changes have been made:

  1. Modularization of the Server Module:

    • The server module has been divided into two parts to avoid side effects like reading configuration files upon import:
      • A declarative module that exports the server class (this is the original server module).
      • A CLI module that handles configuration file reading and server startup.
  2. Splitting the sourcify-chains Module:

    • Similarly, the sourcify-chains module has been split into:
      • A declarative module named sourcify-chain-repository.
      • An imperative module called sourcify-chains.
  3. Decoupling from node-config:

    • All cross-references to node-config have been removed. Configuration files are now read exclusively by the CLI module. All other modules explicitly pass down their configuration options.

This modular approach allows us to integrate Sourcify more seamlessly into our project. We tried to keep the changes at a minimum and barely modified the existing code. The biggest diffs stem from moving imperative code to the cli module.

@kuzdogan kuzdogan self-requested a review September 18, 2024 13:01
@kuzdogan kuzdogan self-assigned this Sep 18, 2024
Copy link
Member

@kuzdogan kuzdogan left a comment

Choose a reason for hiding this comment

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

Couple small things, otherwise lgtm! Thanks a lot.

Tbh I didn't know you could do app.get('var') so this makes things a lot easier.

I made some changes directly myself mainly:

  • getting rid of injecting req.services after learning app.get('services')
  • Renaming repoPath to solcRepoPath and also adding missing solcJsonRepoPaths

Added couple comments for your review. I still need to look at the tests if they'll work fine, let's see.

this.app.set('chainRepository', this.chainRepository);
this.app.set('solc', options.solc);
this.app.set('verifyDeprecated', options.verifyDeprecated);
this.app.set('sessionOptions', options.sessionOptions);
Copy link
Member

Choose a reason for hiding this comment

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

We don't use this anywhere

Suggested change
this.app.set('sessionOptions', options.sessionOptions);

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in ac173f1

Comment on lines 42 to 55

if (
process.env.AWS_REGION === undefined ||
process.env.AWS_ACCESS_KEY_ID === undefined ||
process.env.AWS_SECRET_ACCESS_KEY === undefined
) {
throw new Error(
"AWS credentials not set. Please set them to run the compiler on AWS Lambda.",
);
}

let selectedSolidityCompiler: ISolidityCompiler;
if (config.get("lambdaCompiler.enabled")) {
logger.info("Using lambda solidity compiler with local fallback");
Copy link
Member

Choose a reason for hiding this comment

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

This throwing when no credentials found should only be if lambdaCompiler.enabled, so move it inside if lambdaCompiler.enabled.

Suggested change
if (
process.env.AWS_REGION === undefined ||
process.env.AWS_ACCESS_KEY_ID === undefined ||
process.env.AWS_SECRET_ACCESS_KEY === undefined
) {
throw new Error(
"AWS credentials not set. Please set them to run the compiler on AWS Lambda.",
);
}
let selectedSolidityCompiler: ISolidityCompiler;
if (config.get("lambdaCompiler.enabled")) {
logger.info("Using lambda solidity compiler with local fallback");
let selectedSolidityCompiler: ISolidityCompiler;
if (config.get("lambdaCompiler.enabled")) {
logger.info("Using lambda solidity compiler with local fallback");
if (
process.env.AWS_REGION === undefined ||
process.env.AWS_ACCESS_KEY_ID === undefined ||
process.env.AWS_SECRET_ACCESS_KEY === undefined
) {
throw new Error(
"AWS credentials not set. Please set them to run the compiler on AWS Lambda.",
);
}

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 6447916

@@ -324,74 +337,6 @@ export class Server {
}
}

Copy link
Member

Choose a reason for hiding this comment

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

I imagine some people will assume running dist/server.js. For compatibility, should we add something like below? Would this cause any problems?

if (require.main === module) {
  import "./cli"
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Sprint - Needs Review
Development

Successfully merging this pull request may close these issues.

2 participants