Skip to content
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

Add devnet and stress test #5

Merged
merged 14 commits into from
Oct 19, 2024
Merged

Add devnet and stress test #5

merged 14 commits into from
Oct 19, 2024

Conversation

boray
Copy link
Collaborator

@boray boray commented Sep 19, 2024

Summary

This PR adds interaction scripts that test the zkApp's behavior on Devnet.

  • contracts/src/devnet-interaction.ts: Interacts with the zkApp on Devnet in the same way as interact.ts.
  • contracts/src/stress-test.ts: Tests whether a large number of actions can be settled on Devnet.
  • contracts/src/integration-test.ts: Interacts with the zkApp on Devnet but waits for the settlement module to settle.

@boray boray requested a review from 45930 September 23, 2024 20:42
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@45930 what do you think about moving all test files to test folder and classification of interaction scripts and tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already src/test/, which you can use for this.

I prefer test/ at the top level, but for simplicity in the build, I moved it into src/. I don't mind if you move it, as long as the tests keep passing!

Copy link
Contributor

@45930 45930 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test this case as part of the stress test?

  • Deploy contract
  • Emit 100 actions
  • Exit terminal
  • Reconnect and sync actions from the existing contract

const name = Name.fromString(stringName);
const nr = new NameRecord({
mina_address: addresses.user1,
avatar: Field(0),
url: Name.fromString(stringUrl).packed,
url: Field(0),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a regression because previously we tested an actual value was set, but now we are setting the same as the default value. We should at least use Field(1). Is there a reason not to leave it as it was?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's a mistake. I changed the key type of the mapping to Name from Field. It seems I accidentally defaulted urls to Field(0) while removing .packed s from Names. I reverted this change in the latest commit.

@45930
Copy link
Contributor

45930 commented Sep 24, 2024

Maybe we should also test this case, since there is someone asking about it: https://discord.com/channels/484437221055922177/1287933364007079967

Can we have 2 NameService contracts and 2 offchainStates both running on the same thread?

@boray
Copy link
Collaborator Author

boray commented Sep 24, 2024

Maybe we should also test this case, since there is someone asking about it: https://discord.com/channels/484437221055922177/1287933364007079967

Can we have 2 NameService contracts and 2 offchainStates both running on the same thread?

It may not be relevant to this project. It would be easier to test this in a simpler zkapp.

@boray
Copy link
Collaborator Author

boray commented Sep 24, 2024

Did you test this case as part of the stress test?

  • Deploy contract
  • Emit 100 actions
  • Exit terminal
  • Reconnect and sync actions from the existing contract

no, but it tries to resolve a random name at the end of the cycles. OffchainState calls fetch_actions() under the hood so I am not sure if reconnecting would make a difference.

@45930
Copy link
Contributor

45930 commented Sep 24, 2024

no, but it tries to resolve a random name at the end of the cycles. OffchainState calls fetch_actions() under the hood so I am not sure if reconnecting would make a difference.

I think we need to be absolutely sure this works at least. Handling multiple instances of the same contract is not a requirement, but we can't accidentally brick the contract if our js terminal disconnects. I believe it should work if we reconnect and re-sync the actions, but that should be tested as well.

console.timeEnd('compile contract');

console.time('deploy');
tx = await Mina.transaction({ sender: feepayerAddress, fee: fee }, async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we always redeploy, this transaction gives an invalid fee excess after it's been run once. Can we skip this transaction if the contract is already deployed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we should run the test with a fresh zkApp account. So, I don't think it should continue to run if the zkapp is already deployed. However, reading it from a file is unnecessary and actually cumbersome, so I changed that part to generate the zkApp key randomly

console.timeEnd('set premimum rate');

console.time('settlement proof 1');
let proof = await offchainState.createSettlementProof();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I run this, my code hangs on this line. Here is my contract address:
https://minascan.io/devnet/account/B62qrxnym3P7ixgZPLeJixyX6fgruV9UhEGFF9ViPUHZbDErozZYBuW/txs?type=zk-acc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I fixed this by updating my dependencies, see my other comment in package json.

@@ -26,6 +26,7 @@
"devDependencies": {
"@babel/preset-env": "^7.16.4",
"@babel/preset-typescript": "^7.16.0",
"@types/node": "^22.5.5",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file does not reference o1js directly! We are getting o1js via o1js-pack....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After fixing this locally, my script continues running.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it while debugging the wasm TypeError. After adding it back, it works fine on my end as well. I’ll add it in the next commit.

tx = await Mina.transaction(
{ sender: feepayerAddress, fee: fee },
async () => {
let res = await name_service_contract.resolve_name(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My script failed here since the name was not owned. I think the problem is wait(8); // wait for settlement. This only waits for 8 seconds, which doesn't seem like long enough.

I think the wait function should be upgraded to watch the transaction we created, and only proceed when the transaction is applied. No matter how long we wait, the network may not yet have applied the transaction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, wait function is incorrect. I probably missed that because the random case would have been in the set of settled ones.

Watching the settlement transaction would be the best but I don't think we can do that in o1js.

@boray boray requested a review from 45930 October 17, 2024 22:39
Copy link
Contributor

@45930 45930 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work getting this cleaned up Boray!

@boray boray merged commit 2bcfc49 into main Oct 19, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants