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

implement TestPublicKey util and refactor examples #1574

Merged
merged 12 commits into from
Apr 16, 2024

Conversation

harrysolovay
Copy link
Contributor

@harrysolovay harrysolovay commented Apr 13, 2024

Closes #1516

In addition to solving the false positive / unchecked indexing error of the local blockchain's testAccounts, this PR introduces TestPublicKey, a subclass of PublicKey off of which key (the corresponding PrivateKey) can be accessed. This leads to a reduction in test code boilerplate, as one doesn't need to define the corresponding private key under its own name.

- let feePayerPrivateKey = Local.testAccounts[0]?.privateKey!;
- let feePayerPublicKey = Local.testAccounts[0]?.publicKey!;
+ let [feePayer] = Local.testAccounts

// ...

- await Mina.transaction(feePayerPublicKey, () => {
+ await Mina.transaction(feePayer, () => {
    // ...
- }).sign(feePayerPrivateKey)
+ }).sign(feePayer.key)

For cases in which a non-pre-funded account is needed (for instance, to be used as a contract account), one can use TestPublicKey.random.

const a = Mina.TestPublicKey.random()
a // `TestPublicKey`

const [b, c] = Mina.TestPublicKey.random(2)
b // `TestPublicKey`
c // `TestPublicKey`

Note to reviewers re. naming convention: I addressed some (but not all) of the test naming inconsistencies (#1578).

Copy link
Collaborator

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

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

This is great!

Re naming, "account" for a public key (with attached private key) feels not optimal, there is the distinct concept of an account. So it should stay at "public key" or "address"

src/lib/mina/local-blockchain.ts Outdated Show resolved Hide resolved
tests/artifacts/javascript/on-chain-state-mgmt-zkapp-ui.js Outdated Show resolved Hide resolved
src/lib/mina/token.test.ts Outdated Show resolved Hide resolved
@harrysolovay
Copy link
Contributor Author

Re naming, "account" for a public key (with attached private key) feels not optimal

Can you please add your thoughts in #1578 and I'll fix all occurrences in a follow-up PR?

@harrysolovay harrysolovay changed the title implement TestAccount util and refactor examples implement TestPublicKey util and refactor examples Apr 15, 2024

for (let i = 0; i < 10; ++i) {
for (let i = 0; i < 100; ++i) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@harrysolovay I just realized that this takes 2ms per public key (it does an elliptic curve scaling), so 200ms for 100 accounts feels a bit excessive to do every time we instantiate local blockchain.

Maybe we can create these test accounts lazily and still preserve the same API with some trickery. If not, I'd prefer to create only 20 or so.

All of this can be a separate PR

@mitschabaude mitschabaude merged commit db21aa5 into o1-labs:main Apr 16, 2024
11 of 12 checks passed
@harrysolovay harrysolovay deleted the test-user-types branch April 16, 2024 13:12
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.

Local.testAccounts vs. noUncheckedIndexAccess
2 participants