-
Notifications
You must be signed in to change notification settings - Fork 297
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: contract class must be registered before deployment #10949
Merged
Merged
Changes from 21 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
22752a0
feat: contract class must be registered before deployment
dbanks12 c410d72
Merge branch 'master' into db/enforce-class-registered
dbanks12 8c17959
fix test
dbanks12 a94436e
fix
dbanks12 1a48caa
fix e2e tests
dbanks12 d00b64c
Merge branch 'master' into db/enforce-class-registered
dbanks12 e61d500
merge
dbanks12 359b483
maybe fix
dbanks12 8a35ea1
yarn.lock
dbanks12 447e98d
Merge branch 'master' into db/enforce-class-registered
dbanks12 161e18b
register accounts
dbanks12 f569eb7
fix snapshot
dbanks12 343d427
Merge branch 'master' into db/enforce-class-registered
dbanks12 ee0a397
Merge branch 'master' into db/enforce-class-registered
dbanks12 dcb4cb4
sort accounts & wallets so they match
dbanks12 ac88083
merge
dbanks12 9405f40
Revert "yarn.lock"
dbanks12 629796e
Revert "maybe fix"
dbanks12 b0a937b
Merge branch 'master' into db/enforce-class-registered
dbanks12 9f169f5
fix maybe
dbanks12 697e6e8
fix maybe
dbanks12 b29723b
fix
dbanks12 21b6d79
helper
dbanks12 8ff02f9
simplify
dbanks12 f27f6bf
fix
dbanks12 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,10 +8,12 @@ import { | |
type CompleteAddress, | ||
type DeployL1Contracts, | ||
Fr, | ||
type FunctionCall, | ||
GrumpkinScalar, | ||
type Logger, | ||
type PXE, | ||
type Wallet, | ||
getContractClassFromArtifact, | ||
} from '@aztec/aztec.js'; | ||
import { deployInstance, registerContractClass } from '@aztec/aztec.js/deployment'; | ||
import { type BlobSinkServer, createBlobSinkServer } from '@aztec/blob-sink/server'; | ||
|
@@ -550,13 +552,22 @@ export const addAccounts = | |
|
||
logger.verbose('Simulating account deployment...'); | ||
const provenTxs = await Promise.all( | ||
accountKeys.map(async ([secretKey, signPk]) => { | ||
accountKeys.map(async ([secretKey, signPk], index) => { | ||
const account = getSchnorrAccount(pxe, secretKey, signPk, 1); | ||
const deployMethod = await account.getDeployMethod(); | ||
|
||
// only register the contract class once | ||
let skipClassRegistration = true; | ||
if (index === 0) { | ||
// for the first account, check if the contract class is already registered, otherwise we should register now | ||
if (!(await pxe.isContractClassPubliclyRegistered(account.getInstance().contractClassId))) { | ||
skipClassRegistration = false; | ||
} | ||
} | ||
|
||
const deployMethod = await account.getDeployMethod(); | ||
const provenTx = await deployMethod.prove({ | ||
contractAddressSalt: account.salt, | ||
skipClassRegistration: true, | ||
skipClassRegistration, | ||
skipPublicDeployment: true, | ||
universalDeploy: true, | ||
}); | ||
|
@@ -589,9 +600,16 @@ export async function publicDeployAccounts( | |
) { | ||
const accountAddressesToDeploy = accountsToDeploy.map(a => ('address' in a ? a.address : a)); | ||
const instances = await Promise.all(accountAddressesToDeploy.map(account => sender.getContractInstance(account))); | ||
const batch = new BatchCall(sender, [ | ||
(await registerContractClass(sender, SchnorrAccountContractArtifact)).request(), | ||
...instances.map(instance => deployInstance(sender, instance!).request()), | ||
]); | ||
|
||
const contractClass = getContractClassFromArtifact(SchnorrAccountContractArtifact); | ||
const alreadyRegistered = await sender.isContractClassPubliclyRegistered(contractClass.id); | ||
|
||
const calls: FunctionCall[] = []; | ||
if (!alreadyRegistered) { | ||
calls.push((await registerContractClass(sender, SchnorrAccountContractArtifact)).request()); | ||
} | ||
calls.push(...instances.map(instance => deployInstance(sender, instance!).request())); | ||
|
||
const batch = new BatchCall(sender, calls); | ||
Comment on lines
-592
to
+613
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't batch a call to register the class if it is already registered |
||
await batch.send().wait({ proven: waitUntilProven }); | ||
} |
Oops, something went wrong.
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.
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 had to do something like this instead of what I had done here dcb4cb4 because it is later assumed that
this.wallets
andthis.accounts
are in the same order asthis.keys
. @spalladino there are many places throughout our tests where we do thisEven if all tests pass, I wouldn't be surprised if we run into issues/flakes down the line because of ordering mismatches in other tests. So, my questions are:
pxe.getRegisteredAccounts()
was always matching the order of the account keys as returned fromaddAccounts
. It's not clear to me whypxe.getRegisteredAccounts()
order would no longer match after my changeset.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.
moved it into a helper for now
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.
Can't think of a reason why this is happening now. But I think the easiest thing is to not get the registered accounts from the pxe but from the account managers themselves, so we ensure the order is the same. WDYT?
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'll try that!