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

test(e2e): multiple accounts, same encryption key #1137

Merged
merged 8 commits into from
Jul 24, 2023

Conversation

benesjan
Copy link
Contributor

Description

Fixes #1061

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@benesjan benesjan marked this pull request as draft July 21, 2023 15:18
@benesjan benesjan changed the title Multiple accounts same encryption key e2e test test(e2e): multiple accounts, same encryption key Jul 24, 2023
@benesjan benesjan force-pushed the janb/multiple-accounts-same-encryption-key branch from a1ba34d to 39608d3 Compare July 24, 2023 07:02
@benesjan benesjan marked this pull request as ready for review July 24, 2023 07:43
Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

LGTM!

abi: ContractAbi,
args: any[],
encryptionPrivateKey: Buffer,
useProperKey: boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This parameter is very specific to the account contract e2e tests. I'm fine keeping it in the extracted util for now, but we should rethink this interface later.

Comment on lines 90 to 94
// First wait until the corresponding RPC server has synchronised the account
const isUserSynchronised = async () => {
return await wallet.isAccountSynchronised(owner);
};
await retryUntil(isUserSynchronised, owner.toString(), 5);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious if we actually need this, since we only have 1 rpc server on this test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into the code and it really doesn't seem to be needed here because when tx is mined it is ensured that all the notes were already processed. I removed the check in 0b4aa5a. Thanks

@benesjan benesjan enabled auto-merge (squash) July 24, 2023 16:43
@benesjan benesjan merged commit 4a44add into master Jul 24, 2023
@benesjan benesjan deleted the janb/multiple-accounts-same-encryption-key branch July 24, 2023 16:52
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.

Create an e2e test spending notes from multiple accounts under the same encryption key
2 participants