test: Orderbook TXE tests

[benesjan]

Fixes #14525

We didn't have the Orderbook thoroughly tested as when I implemented it TXE was not yet reasonable. Now it's reasonable so I decided to tackle this. Since AI is known to be good at tests I decided to gave it a try.

AI experiment

This was the prompt + well curated the context window by opening the relevant tabs in Cursor:

write the orderbook tests in noir-projects/noir-contracts/contracts/app/orderbook_contract/src/test.nr

note that the Orderbook contract accepts 2 token contracts on the input so you will need to deploy those first. Use the setup and setup and mint functionality from noir-projects/noir-contracts/contracts/app/token_contract/src/test/utils.nr.

From the orderbook you want to test the setup order and create order functions. There is also already a yarn-project/end-to-end/src/e2e_orderbook.test.ts test but that is written in TypeScript and noit Noir and it tests only the happy path. In the Noir tests I would also like to test unhappy paths. See tests in noir-projects/noir-contracts/contracts/app/token_contract/src/test/transfer_in_private.nr for inspiration on how to structure the Noir tests.

Overall it managed to implement it all. The only thing I need to help it with is that it didn't understand that it needs to refer the token_contract crate when deploying:

env.deploy("Token")
env.deploy("@token_contract/Token")

Would say this was caused by us not having enough examples and me not putting that into a context window.

Other than this it managed to resolve on its own one other issue (tried to mint with an account with token minter permissions).

The only other thing I did is that I nuked quite a few tests it did because it was for example testing that a function call fails when it doesn't have token transfer authwit provided. Felt like this is sufficient to have only in the token tests.

Update on AI

Code quality was not great and had to redo quite a lot of stuff. That could be improved though if we had a proper reference (plenty of the style issues were present in the rest of our codebase).

[benesjan]

• test: Orderbook TXE tests #16451 👈 (View in Graphite)
• next

This stack of pull requests is managed by Graphite. Learn more about stacking.

[nventuro]

Given that the only contract tests that exist are are the ones we wrote, I think it'd be good for these to serve as a sort of reference that can be copied by others (e.g. like you did here). Because of that I'd focus not just on coverage but also overall style etc.

I don't like passing this boolean here for a number of reasons. One, it is a bit obsure (setup(true)), two, the owner doesn't really need to be an account contract since all they'll do is mint, not sign authwits.

[benesjan]

Great point. The bool is ugly.

In this case I solved it by just moving the minter account creation out of the function. It made sense to do because the setup is only 1 line and we've already setup the taker account out of the setup in the full_flow test.

If you like how it looks now I can also update other tests were we have this ugly bool (Token and NFT tests). As you say seems to make sense to do as AI is able to do a useful job if you give it a good reference.

Also just renamed the function to setup as the original name didn't describe it well enough (it also sets up test env and the minter account).