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

Add withdraw excess with test cases #35

Merged
merged 19 commits into from
Nov 14, 2024

Conversation

DogukanGun
Copy link
Contributor

Add withdraw excess with test cases.
This pull request was created for https://app.gib.work/bounties/12adc3ff-7afb-45cd-99c0-399bc43f4807 in an attempt to solve a bounty #27 . Payment for the bounty is immediately sent to the contributor after merge.

program/idl.json Outdated Show resolved Hide resolved
program/idl.json Outdated Show resolved Hide resolved
@DogukanGun
Copy link
Contributor Author

Hi @lorisleiva ,

Thanks for the review, I fixed the parts you mentioned.

program/idl.json Outdated
Comment on lines 44 to 46
"docs": ["Total supply of tokens."],
"docs": [
"Total supply of tokens."
],
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I believe these changes should not exist if things we properly formatted. Have you tried the script that fixes linting/formatting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, did lint and formatting same time. I can change this manually, it is up to you.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, looks like the formatting doesn't affect this file. 😞

Yeah if we could make sure this PR doesn't include unnecessary changes, that'd be great!

Copy link
Member

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

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

There are a few things that needs to be adjusted before I can review this in more details. Please have a look at PR #38 for reference of what we're looking for. 🙏

  • ❌ Add the instruction in idl.json in the right place (ordered by discriminator).
  • ❓ Update getInitializeInstructionsForExtensions.ts accordingly.
  • ❌ Add tests that follow the same conventions as the existing tests.
  • ✅ Ensure the right discriminator is set.
  • ❓ Ensure the multiSigner remaining account is set when applicable.

@DogukanGun
Copy link
Contributor Author

There are a few things that needs to be adjusted before I can review this in more details. Please have a look at PR #38 for reference of what we're looking for. 🙏

  • ❌ Add the instruction in idl.json in the right place (ordered by discriminator).
  • ❓ Update getInitializeInstructionsForExtensions.ts accordingly.
  • ❌ Add tests that follow the same conventions as the existing tests.
  • ✅ Ensure the right discriminator is set.
  • ❓ Ensure the multiSigner remaining account is set when applicable.

Hi @lorisleiva , thanks for the update. I updated the idl.json and also change test case according to reference. Haven't done any changes in getInitializeInstructionsForExtensions.ts because this is not a token extension. Also multiSigner remaining account is set, checked it again.

Copy link
Member

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

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

Thank you! Almost there, just a few more comments before merging! 💪

clients/js/src/generated/programs/token2022.ts Outdated Show resolved Hide resolved
program/idl.json Outdated Show resolved Hide resolved
program/idl.json Show resolved Hide resolved
@DogukanGun
Copy link
Contributor Author

Hi @lorisleiva , thanks for the review. Code is updated according to your review.

Copy link
Member

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

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

Few more changes before merging this in. Then, if the tests are passing, LGTM!

Also please fix formatting to make CI pass. 🙏

program/idl.json Outdated Show resolved Hide resolved
clients/js/test/withdrawExcess.test.ts Outdated Show resolved Hide resolved
program/idl.json Show resolved Hide resolved
@lorisleiva
Copy link
Member

The test is failing due to Attempt to debit an account but found no record of a prior credit.. It looks like some other account need SOL, or perhaps the payer is not set properly on a transaction. Could you please investigate? Once this is fixed, we're good to merge though.

@DogukanGun
Copy link
Contributor Author

The test is failing due to Attempt to debit an account but found no record of a prior credit.. It looks like some other account need SOL, or perhaps the payer is not set properly on a transaction. Could you please investigate? Once this is fixed, we're good to merge though.

Hi @lorisleiva , thanks for the review. Updated the test case.

Copy link
Member

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

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

Perfection! Thank you for your contribution! 🍺

@lorisleiva lorisleiva merged commit 8947647 into solana-program:main Nov 14, 2024
5 checks passed
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.

2 participants