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: aztec start --pxe network=devnet #7705

Merged
merged 13 commits into from
Aug 1, 2024
Merged

feat: aztec start --pxe network=devnet #7705

merged 13 commits into from
Aug 1, 2024

Conversation

spypsy
Copy link
Member

@spypsy spypsy commented Jul 31, 2024

Fixes #7644

Adds the network option to aztec start --pxe
User will need to provide an API key in order to connect to our hosted node

@spypsy spypsy marked this pull request as ready for review July 31, 2024 16:53
Copy link
Contributor

@alexghr alexghr left a comment

Choose a reason for hiding this comment

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

LGTM

yarn-project/aztec-node/src/aztec-node/server.ts Outdated Show resolved Hide resolved
yarn-project/aztec/src/cli/cmds/start_pxe.ts Outdated Show resolved Hide resolved
Comment on lines 88 to 91
address: AztecAddress.fromString(basicContractsInfo[`${key}L2`]),
initHash: Fr.fromString(basicContractsInfo[`${key}L2InitHash`]),
salt: Fr.fromString(basicContractsInfo[`${key}L2Salt`]),
artifact: await getContractArtifact(artifactName, userLog),
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth refactoring how the contracts are saved on static.aztec.network now, before we have to maintain backwards compatibility, so that in the future if we do add new contracts, this code won't need to change. I'm thinking we could save them like this

{
  "testCounter": {
    "address": "...",
    "initializationHash": "....",
    "salt": "....",
    "deployerAddress": "..."
  }
}

Then loading them up in the pxe becomes just an iteration over the top-level keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't something that has to be done in this PR but we should consider doing it soon ™️

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

const devCoin = await TokenContract.deploy(wallet, wallet.getAddress(), 'DevCoin', 'DEV', 18).send().deployed();
const bridge = await TokenBridgeContract.deploy(wallet, devCoin.address, l1Portal).send().deployed();
const devCoin = await TokenContract.deploy(wallet, wallet.getAddress(), 'DevCoin', 'DEV', 18)
.send({ universalDeploy: true })
Copy link
Contributor

Choose a reason for hiding this comment

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

sneaky 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

ah wait this isn't necessary, was trying this before Phil's change on storing initHash & salt

@@ -53,6 +53,7 @@
"@aztec/pxe": "workspace:^",
"@aztec/telemetry-client": "workspace:^",
"@aztec/txe": "workspace:^",
"@aztec/types": "workspace:^",
Copy link
Contributor

Choose a reason for hiding this comment

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

I got bit by it yesterday, this needs a yarn prepare at the root to update aztec/tsconfig.json 🥹

Copy link
Member Author

Choose a reason for hiding this comment

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

I did run it 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

yep it's there 👍

@alexghr
Copy link
Contributor

alexghr commented Aug 1, 2024

Just a heads up that the CI flow only runs on PRs to master so linter errors won't show up until we actually merge devnet to master

spypsy and others added 3 commits August 1, 2024 10:17
Co-authored-by: Alex Gherghisan <[email protected]>
Co-authored-by: Alex Gherghisan <[email protected]>
@spypsy spypsy changed the base branch from devnet to master August 1, 2024 09:24
@spypsy spypsy changed the base branch from master to devnet August 1, 2024 09:24
Copy link
Collaborator

@PhilWindle PhilWindle left a comment

Choose a reason for hiding this comment

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

Just a couple of changes

Comment on lines 88 to 91
address: AztecAddress.fromString(basicContractsInfo[`${key}L2`]),
initHash: Fr.fromString(basicContractsInfo[`${key}L2InitHash`]),
salt: Fr.fromString(basicContractsInfo[`${key}L2Salt`]),
artifact: await getContractArtifact(artifactName, userLog),
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

* Deployed aztec networks & their names.
*/
export enum Network {
DEVNET = 'devnet',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add 'provernet', with the same config as devnet please?

@spypsy spypsy merged commit e82383b into devnet Aug 1, 2024
1 check passed
@spypsy spypsy deleted the spy/start-devnet-pxe branch August 1, 2024 16:32
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.

3 participants