feat: contract class must be registered before deployment #10949
Conversation
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
dbanks12
changed the title
dbanks12
changed the title
dbanks12
commented
Jan 9, 2025
Comment on lines
114
to
137
| this.wallets = await Promise.all(accountManagers.map(a => a.getWallet())); | ||
| this.accounts = await pxe.getRegisteredAccounts(); | ||
| this.wallets = (await Promise.all(accountManagers.map(a => a.getWallet()))).sort((aWallet, bWallet) => { | ||
| const a = aWallet.getAddress().toBigInt(); | ||
| const b = bWallet.getAddress().toBigInt(); | ||
| if (a < b) { | ||
| return -1; | ||
| } else if (a > b) { | ||
| return 1; | ||
| } else { | ||
| return 0; | ||
| } | ||
| }); | ||
| this.accounts = (await pxe.getRegisteredAccounts()).sort((aAccount, bAccount) => { | ||
| const a = aAccount.address.toBigInt(); | ||
| const b = bAccount.address.toBigInt(); | ||
| if (a < b) { | ||
| return -1; | ||
| } else if (a > b) { | ||
| return 1; | ||
| } else { | ||
| return 0; | ||
| } | ||
| }); | ||
| this.wallets.forEach((w, i) => this.logger.verbose(`Wallet ${i} address: ${w.getAddress()}`)); | ||
| }); |
There was a problem hiding this comment.
pxe.getRegisteredAccounts wasn't returning the accounts in the same order as wallets which was causing test failures because there was an assumption that wallets[0] == accounts[0]
dbanks12
commented
Jan 9, 2025
Comment on lines
-592
to
+613
| 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); |
There was a problem hiding this comment.
Don't batch a call to register the class if it is already registered
spalladino
approved these changes
Jan 9, 2025
| const opts = { skipClassRegistration: true }; | ||
| logger.debug(`Trying to deploy contract instance without registering its contract class`); | ||
| await expect(StatefulTestContract.deploy(wallet, owner, owner, 42).send(opts).wait()).rejects.toThrow( | ||
| /Cannot find the leaf for nullifier/, |
There was a problem hiding this comment.
We need better error messages :-P
dbanks12
added
e2e-prover-full
Changes to public function bytecode sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
dbanks12
commented
Jan 10, 2025
Comment on lines
116
to
133
| this.keys = accountKeys; | ||
| const accountManagers = accountKeys.map(ak => getSchnorrAccount(pxe, ak[0], ak[1], SALT)); | ||
| this.wallets = await Promise.all(accountManagers.map(a => a.getWallet())); | ||
| this.accounts = await pxe.getRegisteredAccounts(); | ||
|
|
||
| // This Map and loop ensure that this.accounts has the same order as this.wallets and this.keys | ||
| const registeredAccounts: Map<string, CompleteAddress> = new Map( | ||
| (await pxe.getRegisteredAccounts()).map(acc => [acc.address.toString(), acc]), | ||
| ); | ||
| for (let i = 0; i < this.wallets.length; i++) { | ||
| const wallet = this.wallets[i]; | ||
| const walletAddr = wallet.getAddress().toString(); | ||
| assert( | ||
| registeredAccounts.has(walletAddr), | ||
| `Test account ${walletAddr} not registered, but it should have been`, | ||
| ); | ||
| this.accounts.push(registeredAccounts.get(walletAddr)!); | ||
| } | ||
| this.wallets.forEach((w, i) => this.logger.verbose(`Wallet ${i} address: ${w.getAddress()}`)); |
There was a problem hiding this comment.
I had to do something like this instead of what I had done here dcb4cb4 because it is later assumed that this.wallets and this.accounts are in the same order as this.keys. @spalladino there are many places throughout our tests where we do this
this.wallets = await Promise.all(accountManagers.map(a => a.getWallet()));
this.accounts = await pxe.getRegisteredAccounts();
Even 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:
- Why did this just start happening now? I guess before this PR,
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. - Should something change in the PXE/elsewhere to make sure that these orders match? Or should I just create a test helper function for this?
There was a problem hiding this comment.
moved it into a helper for now
There was a problem hiding this comment.
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?
dbanks12
added a commit
that referenced
this pull request
Jan 10, 2025
I merged #10949 too quickly and missed this one.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves #10832