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

feat(kl-factory): update integration tests #1492

Merged
merged 13 commits into from
Apr 9, 2024

Conversation

Raid5594
Copy link
Collaborator

@Raid5594 Raid5594 commented Mar 25, 2024

What ❔

This PR fixes part of the integration tests.

Why ❔

After addition of bridgehub some of the tests were failing.
Updated the zksync-ethers dependency to latest version, updated paymaster tests, verified that fees, l1, mempool, self-unit and system tests are green, removed l2-weth test.

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk fmt and zk lint.
  • Spellcheck has been run via zk spellcheck.
  • Linkcheck has been run via zk linkcheck.

@Raid5594 Raid5594 changed the title Update integration tests feate(kl-factory): update integration tests Mar 25, 2024
@Raid5594 Raid5594 changed the title feate(kl-factory): update integration tests feat(kl-factory): update integration tests Mar 25, 2024
core/tests/ts-integration/src/context-owner.ts Outdated Show resolved Hide resolved
core/tests/ts-integration/src/env.ts Outdated Show resolved Hide resolved
core/tests/ts-integration/src/system.ts Outdated Show resolved Hide resolved
@@ -54,6 +87,7 @@ describe('Tests for L1 behavior', () => {
alice.requestExecute({
contractAddress: counterContract.address,
calldata,
mintValue: isETHBasedChain ? ethers.BigNumber.from(0) : expectedL2Costs,
Copy link
Contributor

Choose a reason for hiding this comment

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

MintValue should not be zero for not Eth based chains. Just msg.Value can be zero. The difference is mintValue is the amount of baseTokens that is deposited to the L2, while msg.value is on L1 and in ether.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So it is not zero for not Eth based chains. It is zero in case of Eth chains, as in Eth based chain test no mintValue was passed.

Copy link
Member

Choose a reason for hiding this comment

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

I think what @kelemeno means is that mintValue semantics should be simply "deposit value". Whether it is passed as msg.value or a method argument should be decided by the SDK based on the base token.

Copy link
Contributor

@kelemeno kelemeno Apr 4, 2024

Choose a reason for hiding this comment

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

If the eth based chains could be 0 then I don't see why it could not also be 0 for custom base chains?

What I meant that "mintValue" is the some of l2GasFees and the l2Value, and is equal to the total deposited value on L1. And yes, normally when depositing tokens the SDK decides.

Also what I also meant was that even if you don't set mintValue, it will not normally be 0 when the contract is called, as then the tx will fail. So maybe you can set it to 0 here ( depending on the SDK) but potentially that is not ideal. Maybe null/undefined is better.

@kelemeno
Copy link
Contributor

kelemeno commented Apr 2, 2024

Also linting is not running, you can run it with zk fmt, and zk lint

@Raid5594 Raid5594 force-pushed the feat/update-integration-tests branch 2 times, most recently from 964ac80 to 7b85dc7 Compare April 3, 2024 13:28

await sleep(8);
// console.log('balances after deposit: ');
// console.log('alice.getBalance(): ', await alice.getBalance());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you forgot to delete these comments, if however you want to print them then use the debugging reporter I think ( but not console logs)

@Raid5594 Raid5594 force-pushed the feat/update-integration-tests branch from 2aa1f43 to 0efd1b7 Compare April 5, 2024 16:44
@Raid5594 Raid5594 force-pushed the feat/update-integration-tests branch from 115755e to bac9345 Compare April 9, 2024 17:08
@kelemeno kelemeno merged commit 20574f7 into kl-factory Apr 9, 2024
22 of 41 checks passed
@kelemeno kelemeno deleted the feat/update-integration-tests branch April 9, 2024 17:29
@Raid5594 Raid5594 restored the feat/update-integration-tests branch April 12, 2024 09:56
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.

None yet

3 participants