-
Notifications
You must be signed in to change notification settings - Fork 1
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
Select contracts implementing the correct trait from the Clarinet project for argument generation #88
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting work on the draft implementation so far! Left some comments.
The next steps I see are:
- Create (perhaps parameterized?) test(s) based on the draft/example in
property.ts
. - Add as many more test cases as possible to expand coverage.
- Tweak the draft implementation to ensure all tests pass.
- (Once tests provide decent coverage, we'll consider refactoring with a
parsec-style library.)
fbbb156
to
74a086b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left questions and comments at the abstraction level. Let's make sure we get those right before proceeding.
e92b62a
to
ae0d20c
Compare
c65fc8a
to
c189840
Compare
85dce3b
to
1d76720
Compare
2b837d9
to
bef6767
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is massive 🥇 After refactoring simnet
and coming up with candidate alternatives for "enrich" this should be ready for review.
This commit adds a utility function called `extractProjectTraitImplementations`. It creates a pre-defined record that maps all the contracts in the simnet instance to their implemented traits. With this change, Rendezvous no longer needs to iterate through all contracts during argument generation. Instead, the pre-defined record is passed to all utilities that return arbitraries.
This commit addresses #88 (comment).
Gigantic cleaning session happened here. At least a couple things to decide on before merging:
Ideally, we should have two trait implementations for the same trait, so Rendezvous can randomly pick one of them for any trait reference parameter. |
Any contract that shows a real-world use case and stays under 100 lines.
I'd say both: a local handmade one and one added as a requirement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a massive and tricky-to-implement PR. It's been carefully crafted, and I want to leave an approval noting that it's one of the most well-crafted PRs (of this size) I've reviewed in over a decade.
I've left a few comments, but they aren't blockers. It would be nice to address them, but we could merge and handle those separately if needed.
One thing to consider for this PR is whether to keep the full commit history or squash it into a single commit. Keeping the history preserves individual commit context, which can be helpful for debugging and provides a detailed timeline of changes. On the other hand, it can make the history noisier with too many intermediate commits.
Squashing into a single commit results in a cleaner, more concise history, making it easier to track high-level changes. It also simplifies git revert
if we need to roll back due to blockers, which is especially useful since we're still < v1 and can afford breaking changes. However, squashing does lose the detailed commit trail, which can sometimes be useful for debugging.
As Linus Torvalds put it: "I want clean history, but that really means (a) clean and (b) history." (Source: https://www.mail-archive.com/[email protected]/msg39091.html)
.../.cache/requirements/SP4SZE494VC2YC5JYG7AYFQ44F5Q4PYV7DVMDPBG.sip-010-trait-ft-standard.clar
Outdated
Show resolved
Hide resolved
This commit adds the following to the example project: - `ft-transfer-many`: A contract that imports a trait and references it in a function parameter. - `rendezvous-token`: A contract that implements the referenced trait. Additionally, it includes an invariant and a property test for the ft-transfer-many contract to demonstrate the random selection of trait implementations in action.
33432a1
to
c76fa69
Compare
f70fdd0
to
669df31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR adds support for arbitrary selection of
trait reference
parameters. The new logic for handling trait implementations in trait reference parameters is as follows:To test contracts with trait reference parameters, users must select at least one trait implementation for the referenced trait and add it to their Clarinet project, either as a direct contract or a project requirement.
This PR resolves #65.