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

contracts: fix intermittent failures in auth.ts tests #375

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

CedarMist
Copy link
Member

fixes: #361

Also fixes typescript warnings in auth.ts

@CedarMist CedarMist added contracts Pull requests that update sapphire-contracts bug tests Relating to tests labels Sep 3, 2024
@CedarMist CedarMist self-assigned this Sep 3, 2024
Copy link

netlify bot commented Sep 3, 2024

Deploy Preview for oasisprotocol-sapphire-paratime canceled.

Name Link
🔨 Latest commit a4beb2a
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-sapphire-paratime/deploys/66fad74aaa73a00008a39bc2

@CedarMist CedarMist force-pushed the CedarMist/auth-intermittent-failure branch from 34e196f to 0a81358 Compare September 3, 2024 12:12
@CedarMist CedarMist marked this pull request as ready for review September 3, 2024 12:17
@CedarMist CedarMist requested a review from aefhm September 3, 2024 15:46
@CedarMist CedarMist mentioned this pull request Sep 3, 2024
contracts/test/auth.ts Outdated Show resolved Hide resolved
contracts/test/auth.ts Outdated Show resolved Hide resolved
@CedarMist
Copy link
Member Author

Well... F...

This is still running into the problem! We're waiting for bearer token to expire, push a transaction through... And it still thinks the bearer token is valid?

  1) Auth
       Should call authenticated method:
     TypeError: Expected a valid transaction hash, but got 'Very secret message'
      at onSuccess (/home/runner/work/sapphire-paratime/sapphire-paratime/node_modules/.pnpm/@[email protected]_@[email protected][email protected]_djiantmfwzd4h5mvs2d3iacurm/node_modules/@nomicfoundation/hardhat-chai-matchers/src/internal/reverted/reverted.ts:32:17)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)
      at async Context.<anonymous> (test/auth.ts:165:5)

@CedarMist
Copy link
Member Author

@matevz Any idea why this test is still failing intermittently?

@lukaw3d
Copy link
Member

lukaw3d commented Sep 4, 2024

This is still running into the problem

I tried repeating the test
https://github.com/oasisprotocol/sapphire-paratime/actions/runs/10692954396/job/29642295984
failed 6/10 repeats

I tried double await siweAuthTests.doNothing();
https://github.com/oasisprotocol/sapphire-paratime/actions/runs/10693129388/job/29642755780
failed 1/10 repeats

I tried await delay(6000);
https://github.com/oasisprotocol/sapphire-paratime/actions/runs/10694436081/job/29646226736
it succeeded 20/20 repeats

@lukaw3d
Copy link
Member

lukaw3d commented Sep 4, 2024

https://github.com/oasisprotocol/sapphire-paratime/actions/runs/10692954396/job/29642295984
also randomly failed "Delegate then begin Undelegate (with receipts)"

expect(await ethers.provider.getBalance(await contract.getAddress())).eq(
initialContractBalance + parseEther('100'),
);

(I tried repeating it 20 times but it just started failing differently after 10th success 🤷 https://github.com/oasisprotocol/sapphire-paratime/actions/runs/10694614566/job/29646662356 )

Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

Nice catch, thanks.

contracts/test/auth.ts Show resolved Hide resolved
@matevz
Copy link
Member

matevz commented Sep 4, 2024

@matevz Any idea why this test is still failing intermittently?

#375 (comment)

@CedarMist CedarMist force-pushed the CedarMist/auth-intermittent-failure branch from 49b483d to bd482a3 Compare September 4, 2024 11:15
@CedarMist
Copy link
Member Author

CedarMist commented Sep 4, 2024

So, even if I wait for the tx receipt from doNothing, I still get intermittent failures.

Ohh.. I know why, because block timestamps are in seconds, we're waiting less than a second. If I change it to wait at least 1 second then rounding won't cause it to succeed when it should fail.

@CedarMist CedarMist force-pushed the CedarMist/auth-intermittent-failure branch 2 times, most recently from 20840a9 to 206a431 Compare September 4, 2024 11:31
@CedarMist
Copy link
Member Author

CedarMist commented Sep 4, 2024

So to summarize:

  • Wasn't waiting for tx to be mined by waiting for receipt
  • Was providing milliseconds, which rounded to seconds
  • On-chain time is in seconds
  • Wasn't waiting an extra second, to account for possible round-up

Have now run this locally 20 times in a row with no failures.

Yet CI failed again...

Will revisit this tomorrow or Friday. For now we can continue re-running failed CI tests if we encounter this bug.

@matevz
Copy link
Member

matevz commented Sep 4, 2024

Interesting. Maybe the new block wasn't propagated yet to the oasis web3 gw (and postgres db) and it used the old block number when performing the query. Did you try adding sleep after waiting for the receipt and before making a query?

@CedarMist
Copy link
Member Author

I'll add in more debugging to check:

  • timestamp in SIWE message
  • timestamp, block number & epoch returned from view call
  • timestamp of block etc.

@CedarMist CedarMist force-pushed the CedarMist/auth-intermittent-failure branch from 206a431 to bb26a85 Compare September 23, 2024 04:36
@CedarMist CedarMist force-pushed the CedarMist/auth-intermittent-failure branch from bb26a85 to a4beb2a Compare September 30, 2024 16:52
@CedarMist CedarMist merged commit 825c8ef into main Sep 30, 2024
10 checks passed
@CedarMist CedarMist deleted the CedarMist/auth-intermittent-failure branch September 30, 2024 17:03
github-actions bot added a commit that referenced this pull request Sep 30, 2024
…edarMist/auth-intermittent-failure

contracts: fix intermittent failures in auth.ts tests 825c8ef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug contracts Pull requests that update sapphire-contracts tests Relating to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intermittent failure in contracts/test/auth.ts
4 participants