test: dual pxe tests #12
Conversation
ro run this test you'll need to run two separate of the aztec sandbox First you need to run the sandbox normally `aztec start --sandbox`, then you need to run a PXE instance pointing to the previously started sandbox: `aztec start --port 8081 --pxe --pxe.nodeUrl=http://host.docker.internal:8080`
| // const transfer2Tx = await token.withWallet(bob).methods.transfer_to_private(bob.getAddress(), wad(5)).send().wait({ | ||
| // debug: true | ||
| // }) |
There was a problem hiding this comment.
transfer_to_private is public -> private, and bob has now a private balance (since Alice transfer is private->private), can change it for transfer_in_private(bob.getAddress(), bob.getAddress(), wad(5), 0) and test will pass
There was a problem hiding this comment.
I think this was due to a confusion on my part but I think there's something weird happening regarding the available notes in each PXE. I'll review it and probably ask you to discuss it later.
| expect(await token.methods.balance_of_private(address).simulate()).toBe(privateBalance); | ||
| }; | ||
|
|
||
| const wad = (n: number = 1) => AMOUNT * BigInt(n); |
There was a problem hiding this comment.
wad is confusing, why not just use 10n instead of wad(10) -> 10 * 1000n?
There was a problem hiding this comment.
would _amount(n: number = 1): BigInt be clearer? (added underscore to avoid conflicts with variables named amount)
but to answer your question, because:
- centralized conversion, just pick a number and it will be converted to bigint without fuss (or don't and use default value)
- short and expressive (although I agree it could have a name change!)
- it's shorter than the multiplication expression
ci: override sandbox docker config to run a secondary PXE --------- Co-authored-by: zorzal <zorzal@defi.sucks>
📝 WalkthroughWalkthroughThe pull request updates several key components. In the continuous integration workflow, the GitHub Actions configuration is modified to trigger on push and pull_request events for the main branch only, with an added 30-minute timeout for a specific job. The workflow now separates the Aztec version setup from starting the sandbox, which is launched using Docker Compose in the background, and introduces a dedicated step for executing JavaScript tests via Jest. In the container orchestration configuration, a new service named "alternative-pxe" is added, featuring Docker image details, environment variable settings, volume mounts, and service dependencies. Additionally, the test suite for the token contract is expanded with new functions for deploying token and escrow contracts, revised variable types, and additional tests that cover multi-PXE interactions, including scenarios for token minting, transferring, and balance verification. Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/token_contract/src/test/main.test.ts (3)
30-37: Function createPXE for dynamic multi-PXE.
Flexible approach for running PXE on distinct ports.Consider adding a brief console or logger statement to output the final URL for easier debugging.
43-49: deployToken helper function with console.log.
Logging the contract address is helpful, though you may want to unify logs using a single shared logger instead of console.- console.log('Token contract deployed at', contract.address); + // For consistent logging: + // logger.info('Token contract deployed at', contract.address);
51-66: deployEscrow concurrent registration.
The approach using Promise.all is efficient.• Consider calling computePartialAddress(escrowInstance) once outside the loop, since it is constant.
• Similarly, to keep logging consistent, you could integrate the existing logger instead of console.log..github/workflows/tests.yaml (3)
30-30: Trailing spaces detected.
Please remove trailing spaces to pass lint checks.- VERSION=0.72.1 aztec-up + VERSION=0.72.1 aztec-up🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 30-30: trailing spaces
(trailing-spaces)
31-33: Splitting sandbox startup into a separate step.
Good approach for clarity.Consider using “docker compose up -d” if you prefer a detached mode instead of backgrounding with an ampersand.
47-49: “Run nr tests” naming.
The short name “nr” might be ambiguous.You could rename the step to “Run Noir tests” for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/tests.yaml(3 hunks)docker-compose.override.yml(1 hunks)src/token_contract/src/test/main.test.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.ts`: Review TypeScript files for adherence to the fo...
**/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoidany; useunknownwhen necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have anexternal.tsfile explicitly listing public exports.
- Usebigintas-is; cast toNumberonly when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
src/token_contract/src/test/main.test.ts
`**/*.test.ts`: Review the unit test files with the followin...
**/*.test.ts: Review the unit test files with the following guidelines:
- Avoid using the word "should" in test descriptions.
- Ensure descriptive test names convey the intent of each test.
- Validate adherence to the Mocha/Chai/Jest/Vitest/Cypress test library best practices.
- Be concise and avoid overly nitpicky feedback outside of these best practices.
src/token_contract/src/test/main.test.ts
🪛 YAMLlint (1.35.1)
.github/workflows/tests.yaml
[warning] 9-9: wrong indentation: expected 8 but found 6
(indentation)
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[warning] 50-50: too many blank lines
(1 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: setup-and-run
🔇 Additional comments (13)
src/token_contract/src/test/main.test.ts (9)
1-1: New import for Transfer event looks good.
No issues detected. This import aligns with the test usage of Transfer.
15-18: Additional imports for Aztec features are correctly introduced.
These imports (AztecAddress, AccountWalletWithSecretKey, Wallet, and UniqueNote) support multi-PXE and private/public balance testing.
20-26: Added imports for account creation and key derivation.
These are appropriate libraries for testing flows involving derived keys and partial addresses.
27-29: Escrow contract imports.
Including EscrowContract and EscrowContractArtifact is consistent for testing escrow functionality.
39-41: Lightweight wrapper around createPXE.
setupSandbox simply delegates to createPXE. This is fine as a convenience method.
70-70: Type changed to AccountWalletWithSecretKey[].
This change makes sense for private note and secret-key-based operations.
90-90: Mapping wallet arrays to addresses.
This line properly captures each wallet’s complete address, no issues.
98-98: deployToken invoked in beforeEach.
Ensures each test runs with a freshly deployed contract.
437-704: Multi PXE and escrow test suite.
The new “describe('Multi PXE', ... )” block provides comprehensive coverage for multi-PXE interactions, public/private transfers, and escrow flows.• Consider unifying logs with a shared logger.
• At line 524, a “wad” helper is used. A previous reviewer noted that the “wad” naming could be confusing.• The escrow tests are nicely detailed; adding corner-case tests for multiple partial withdrawals or advanced edge scenarios could further strengthen coverage.
docker-compose.override.yml (1)
1-19: New alternative-pxe service definition.
Helps run an additional PXE instance for multi-PXE testing.• Ensure the “LOG_LEVEL” variable’s syntax (with semicolon) works correctly.
• The “TODO” for health checks is important; consider implementing a proper readiness check so the container waits for the main sandbox..github/workflows/tests.yaml (3)
6-6: Pull request branch trigger restricted to 'main.'
Verify that removing the dev branch trigger is intentional.
10-10: Job timeout set to 30 minutes.
Ensure 30 minutes is sufficient for your growing test suite.
44-45: Dedicated “Run js tests” step.
This step is explicit and preserves clarity for integration tests.
| @@ -405,3 +434,271 @@ describe('Token', () => { | |||
| expect(await token.methods.balance_of_private(bob.getAddress()).simulate()).toBe(AMOUNT); | |||
| }, 300_000); | |||
| }); | |||
|
|
|||
| describe('Multi PXE', () => { | |||
There was a problem hiding this comment.
can we move this one into perhaps:
from /src/token_contract/src/test
to /src/test/token_contract.test.ts
and move this part into /src/test/escrow_contract.test.ts?
There was a problem hiding this comment.
I planned to do it once I updated to the latest version of the escrow.
but yes, I agree on move it out of the token contract directory.
To run these tests you need to run two separate of the aztec sandboxes
First you need to run the sandbox normally
aztec start --sandboxThen you need to run a PXE instance pointing to the previously started sandbox:
aztec start --port 8081 --pxe --pxe.nodeUrl=http://host.docker.internal:8080Currently blocked because although I've sent the tokens to the escrow contract I'm unable to withdraw from it.
Summary by CodeRabbit