-
Notifications
You must be signed in to change notification settings - Fork 266
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: Create standalone instances of Aztec Node and RPC Server #2486
Conversation
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
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.
Now that was quite a PR. I think we should be able to simplify much of it (and remove many of the new dependencies) by going back to having NodeInfo and L1Addresses as just interfaces rather than classes. And I'd also consider moving terraform templates specific to our own infra out of the public monorepo.
ENTRYPOINT ["yarn", "start"] |
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 have been using entrypoint pretty liberally, but I'd push for having just yarn
as entrypoint
, and start
as its command
. Not really important though unless we try to make this consistent across the board.
}, | ||
{ | ||
"name": "BOOTSTRAP_NODES", | ||
"value": "/ip4/44.201.46.76/tcp/40400/p2p/12D3KooWGBpbC6qQFkaCYphjNeY6sV99o4SnEWyTeBigoVriDn4D" |
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.
Where's this IP coming from? Should we make it a param, or add a comment?
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.
Committed by accident, will revert
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 template feels pretty coupled with our (as in Aztec Labs') own infrastructure, and not really usable by anyone outside the org. Maybe we should move it to a separate private repo, and leave here only templates reusable by the community?
"name": "SEARCH_START_BLOCK", | ||
"value": "15918000" |
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.
Why this block in particular?
* Creates the sandbox from provided config and deploys any initial L1 and L2 contracts | ||
* @param isP2PSandbox - Flag indicating if this sandbox is to connect to a P2P network | ||
*/ | ||
async function createAndDeploySandbox(isP2PSandbox: boolean) { |
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.
Is the isP2PSandbox
flavor used anywhere?
"assert": "^2.1.0", | ||
"browserify-zlib": "^0.2.0", | ||
"buffer": "^6.0.3", | ||
"crypto-browserify": "^3.12.0", | ||
"jest": "^29.5.0", | ||
"jest-mock-extended": "^3.0.3", | ||
"process": "^0.11.10", | ||
"querystring-es3": "^0.2.1", | ||
"resolve-typescript-plugin": "^2.0.1", | ||
"stream-browserify": "^3.0.0", | ||
"stream-http": "^3.2.0", |
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.
From what you mentioned earlier, I understand these are the new deps needed to please the webpack god. It seems these are required because we now have a dependency on aztec/ethereum
, which is only needed to grab the L1ContractAddresses
type.
Maybe we can just move the L1ContractAddresses
to aztec/types
, so we don't introduce the new dependency (since we want js to be as slim as possible), and don't have these new issues with webpack?
@@ -32,7 +33,7 @@ export const createAztecRpcClient = (url: string, fetch = makeFetch([1, 2, 3], t | |||
NotePreimage, | |||
AuthWitness, | |||
}, | |||
{ Tx, TxReceipt, L2BlockL2Logs }, | |||
{ Tx, TxReceipt, L2BlockL2Logs, NodeInfo }, |
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.
Actually, do we need to add NodeInfo
here? If we keep it as an interface instead of a type, and we initialise it as a plain object, doesn't the automagic (de)serialiser go into each field and serialise that, without having to register the class here?
If we can remove it and just keep NodeInfo as a type or interface (and not a class) we can roll back the new dependency on aztec/ethereum completely.
/** | ||
* Helper function that retrieves the addresses of configured deployed contracts. | ||
*/ | ||
function retrieveL1Contracts(config: AztecNodeConfig, account: Account): Promise<DeployL1Contracts> { |
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.
Took me a while to understand that this function does not retrieve the addresses, it just creates the wallet and public clients and bundles everything together. I'd rename the function, or rather remove it in favor of a helper that just assembles the wallet and public clients (and we reuse that for deployL1Contracts
as well), and use it along with the config
to create the DeployL1Contracts
object directly.
* Does not start any HTTP services nor populate any initial accounts. | ||
* @param config - Optional Sandbox settings. | ||
*/ | ||
export async function createP2PSandbox() { |
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.
Is the only difference with createSandbox
that this one does not deploy the L1 contracts? If so, I'd go back to a single createSandbox
function, and add an option into the SandboxConfig
like skipDeployL1
that controls that behavior. Otherwise, it feels like we have a lot of duplication between createP2PSandbox
and createSandbox
.
@@ -31,6 +31,7 @@ | |||
}, | |||
"dependencies": { | |||
"@aztec/circuits.js": "workspace:^", | |||
"@aztec/ethereum": "workspace:^", |
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.
aztec/ethereum
brings a bunch of annoying L1 dependencies (such as our l1-artifacts) that we shouldn't need to pull in everywhere, given types
is imported everywhere. It'd be great if we can remove this new dependency.
I think we should be able to roll back L1ContractAddresses to be just a type or interface (and define it within types
), and let the converter work its magic by working on a plain object:
aztec-packages/yarn-project/foundation/src/json-rpc/convert.test.ts
Lines 62 to 66 in 471159b
test('converts a plain object', () => { | |
const obj = { a: 10, b: [20, 30], c: 'foo' }; | |
const cc = new ClassConverter(); | |
expect(convertFromJsonObj(cc, convertToJsonObj(cc, obj))).toEqual(obj); | |
}); |
This PR addresses the comments of PR [2486](#2486). The feature set here is: 1. Some refactoring of packages dependencies and l1 contract address configuration. 2. Entrypoints to start standalone instances of Aztec Node and Aztec RPC Server. 3. Some initial terraform for Aztec Node deployments. # Checklist: Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge. - [ ] If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag. - [ ] I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code. - [ ] Every change is related to the PR description. - [ ] I have [linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue) this pull request to relevant issues (if any exist).
This PR addresses the comments of PR [2486](AztecProtocol/aztec-packages#2486). The feature set here is: 1. Some refactoring of packages dependencies and l1 contract address configuration. 2. Entrypoints to start standalone instances of Aztec Node and Aztec RPC Server. 3. Some initial terraform for Aztec Node deployments. # Checklist: Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge. - [ ] If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag. - [ ] I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code. - [ ] Every change is related to the PR description. - [ ] I have [linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue) this pull request to relevant issues (if any exist).
This PR enables both Aztec Node and RPC Servers to be created as standalone services with JSON PRC endpoints.
Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.