-
Notifications
You must be signed in to change notification settings - Fork 388
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
base: staging
Are you sure you want to change the base?
Reusable server module #1637
Conversation
There was a problem hiding this 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 learningapp.get('services')
- Renaming
repoPath
tosolcRepoPath
and also adding missingsolcJsonRepoPath
s
Added couple comments for your review. I still need to look at the tests if they'll work fine, let's see.
services/server/src/server/server.ts
Outdated
this.app.set('chainRepository', this.chainRepository); | ||
this.app.set('solc', options.solc); | ||
this.app.set('verifyDeprecated', options.verifyDeprecated); | ||
this.app.set('sessionOptions', options.sessionOptions); |
There was a problem hiding this comment.
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
this.app.set('sessionOptions', options.sessionOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in ac173f1
services/server/src/server/cli.ts
Outdated
|
||
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"); |
There was a problem hiding this comment.
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
.
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.", | |
); | |
} |
There was a problem hiding this comment.
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 { | |||
} | |||
} | |||
|
There was a problem hiding this comment.
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"
}
Tests are passing if you want to check the comments @RaoulSchaffranek I'll handle the conflicts and the rest |
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:
Modularization of the Server Module:
Splitting the sourcify-chains Module:
sourcify-chains
module has been split into:sourcify-chain-repository
.sourcify-chains
.Decoupling from node-config:
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.