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

Starknet foundry unit testing #9

Closed

Conversation

aagbotemi
Copy link

This is the implementation of Starknet foundry unit testing using snforge

@tekkac
Copy link
Contributor

tekkac commented Apr 16, 2024

run scarb fmt on the code :)

@aagbotemi
Copy link
Author

I ran scarb fmt and scarb build on the code, and it was successful.

Copy link
Contributor

@tekkac tekkac 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 your patience on the review.
Some additional rework is needed.

  • mint test on the token aren't needed, what we'd like to unit test is the EIP7498 methods.
  • some approvals aren't needed.
  • most post method call checks are missing. <-- We want to check that the logic of the redemption is working here.
  • boundary checks are missing. <-- What happens when no token are left to redeem, etc.

Please try to not reuse code and import test methods you need if they exist.

Hope this is helpful.

tests/erc7498_unit_test.cairo Outdated Show resolved Hide resolved
tests/erc7498_unit_test.cairo Outdated Show resolved Hide resolved
tests/erc7498_unit_test.cairo Outdated Show resolved Hide resolved
tests/erc7498_unit_test.cairo Outdated Show resolved Hide resolved
tests/erc7498_unit_test.cairo Show resolved Hide resolved
tests/erc7498_unit_test.cairo Outdated Show resolved Hide resolved
tests/erc7498_unit_test.cairo Outdated Show resolved Hide resolved
tests/erc7498_unit_test.cairo Outdated Show resolved Hide resolved
tests/erc7498_unit_test.cairo Show resolved Hide resolved
tests/erc7498_unit_test.cairo Outdated Show resolved Hide resolved
@aagbotemi aagbotemi requested a review from tekkac May 12, 2024 12:29
@aagbotemi
Copy link
Author

Hi @tekkac, I've done the unit testing and resolved the issues you raised.

@tekkac
Copy link
Contributor

tekkac commented Jun 24, 2024

I'm closing this one as it has been superseded by the current Seaport issues.
Checks after executing the methods are still missing and it's not mergable so I'll close the PR.
If you're still interested in contributing, you could check with the contributors on the current Seaport issues if they want to split and pick up some of their work.

@tekkac tekkac closed this Jun 24, 2024
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