Skip to content
This repository was archived by the owner on Jan 24, 2025. It is now read-only.

fix and improve duplicate-mutable-accounts lesson #531

Closed

Conversation

wuuer
Copy link
Contributor

@wuuer wuuer commented Sep 27, 2024

Problem

Magic number found on space calculation.
"BorshDeserialize" and "BorshSerialize" are outdated.
Test descriptions are not clear enough.

Summary of Changes

Use InitSpace to calculate space needed for accounts.
Replace "BorshDeserialize" and "BorshSerialize" with "AnchorDeserialize" and "AnchorSerialize".
Make test descriptions more clear.

Also, I made a PR for solana-duplicate-mutable-accounts starter branch and a PR for solana-duplicate-mutable-accounts solution branch
which must be synced with this PR.

Jeff Wood and others added 5 commits September 27, 2024 09:58
….InitSpace.html) to calculate space needed for accounts.

Delete Unnecessary parameters found in the test typescript.
Consistently use "rpc()" as sending transactions in the test typescript.
….InitSpace.html) to calculate space needed for accounts.

Replace "BorshDeserialize" and "BorshSerialize" with "AnchorDeserialize" and "AnchorSerialize".
Make test descriptions more clear.
Comment on lines +290 to +291
...
it("Invoke insecure instruction with the same player should be successful", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
...
it("Invoke insecure instruction with the same player should be successful", async () => {
...
it("Invokes insecure instruction with the same player successfully", async () => {

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@mikemaccana mikemaccana left a comment

Choose a reason for hiding this comment

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

Thanks for this! We can get this in easily, just needs modifications to meet the contributing guidelines but they're all small fixes.

Comment on lines +290 to +291
...
it("Invoke insecure instruction with the same player should be successful", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +381 to +382
const p1 = await program.account.playerState.fetch(playerOne.publicKey);
const p2 = await program.account.playerState.fetch(playerTwo.publicKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

const p1 = await program.account.playerState.fetch(playerOne.publicKey);
const p2 = await program.account.playerState.fetch(playerTwo.publicKey);
assert.equal(JSON.stringify(p1.choice), JSON.stringify({ rock: {} }));
assert.equal(JSON.stringify(p2.choice), JSON.stringify({ scissors: {} }));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do this without the JSON.stringify()? assert.deepEqual() or similar?

})
.rpc()
.rpc();
} catch (err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

See https://github.com/solana-foundation/developer-content/blob/main/CONTRIBUTING.md#jsts

catch (thrownObject) {
  const error = thrownObject as Error
  ... 
}

Push your code to GitHub and
[tell us what you thought of this lesson](https://form.typeform.com/to/IPH0UGz7#answers-lesson=9b759e39-7a06-4694-ab6d-e3e7ac266ea7)!
</Callout>
---
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be a second copy of the lesson!

@mikemaccana mikemaccana changed the title fix and improve duplicate-mutable-accounts course fix and improve duplicate-mutable-accounts lesson Oct 17, 2024
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. Remove stale label or comment or this will be closed in 7 days.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants