Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Configure sandbox for network #2818
feat: Configure sandbox for network #2818
Changes from 14 commits
ed83c3c
ba8165b
c7a5e39
d9cd0be
35c65e0
40fe38d
c8426ed
ca7a3a3
085f787
da85b53
e2f3a02
9ff3ee2
e556f4f
76b42e7
0e8cee0
f54a1c0
5c3ad92
ff613fb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 API kind of sucks, you shouldn't need to keep the
deployMethod
around just to get the deployment address. Maybe we should inject it intoyarn-project/aztec.js/src/contract_deployer/deploy_sent_tx.ts
so you can query it from thetx
object directly?(not a note for this PR, but more of a comment for something we need to change elsewhere)
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.
Yeah I thought it was a bit rubbish. I went ahead and made the change.
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 remove the chain id from the configuration? It's usually a good practice to include the chain id as a safeguard to ensure you don't accidentally connect to the wrong network by entering an incorrect RPC URL.
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.
That's a good point. I added the chain Id configuration back in and validate it on Aztec Node creation.