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

Select contracts implementing the correct trait from the Clarinet project for argument generation #88

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

BowTiedRadone
Copy link
Collaborator

@BowTiedRadone BowTiedRadone commented Jan 13, 2025

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:

  • If a trait reference parameter is found in public functions or invariants during invariant testing, or in test functions during property testing, Rendezvous will scan the user's Clarinet project for a contract that implements the referenced trait.
  • If trait implementations are found for that specific trait reference, they will be added to a selection pool. Rendezvous uses fast-check to randomly select one of them for each run, ensuring seed-based randomness.

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.

Copy link
Member

@moodmosaic moodmosaic left a 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.)

citizen.tests.ts Outdated Show resolved Hide resolved
property.ts Outdated Show resolved Hide resolved
property.ts Outdated Show resolved Hide resolved
shared.types.ts Show resolved Hide resolved
shared.types.ts Outdated Show resolved Hide resolved
@BowTiedRadone BowTiedRadone force-pushed the feat/project-trait-selection branch from fbbb156 to 74a086b Compare January 17, 2025 21:47
Copy link
Member

@moodmosaic moodmosaic left a 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.

ast1737030528990.json Outdated Show resolved Hide resolved
example/contracts/trait.clar Outdated Show resolved Hide resolved
property.ts Outdated Show resolved Hide resolved
property.ts Outdated Show resolved Hide resolved
@BowTiedRadone BowTiedRadone force-pushed the feat/project-trait-selection branch from 74a086b to e92b62a Compare January 20, 2025 18:16
@BowTiedRadone BowTiedRadone force-pushed the feat/project-trait-selection branch from e92b62a to ae0d20c Compare January 20, 2025 21:25
invariant.ts Outdated Show resolved Hide resolved
invariant.ts Outdated Show resolved Hide resolved
property.ts Outdated Show resolved Hide resolved
property.ts Outdated Show resolved Hide resolved
property.ts Outdated Show resolved Hide resolved
traits.tests.ts Outdated Show resolved Hide resolved
traits.tests.ts Outdated Show resolved Hide resolved
traits.tests.ts Outdated Show resolved Hide resolved
traits.tests.ts Outdated Show resolved Hide resolved
traits.tests.ts Show resolved Hide resolved
This commit moves the trait test inputs from the fixtures floder directly to
`traits.tests.ts` in a `testInputs` object. This is done to make it easier to
navigate the test inputs and to make it easier to add new test inputs in the
future.
@BowTiedRadone BowTiedRadone force-pushed the feat/project-trait-selection branch from c65fc8a to c189840 Compare January 21, 2025 14:13
@BowTiedRadone BowTiedRadone force-pushed the feat/project-trait-selection branch from 85dce3b to 1d76720 Compare January 21, 2025 14:26
@BowTiedRadone BowTiedRadone force-pushed the feat/project-trait-selection branch from 2b837d9 to bef6767 Compare January 21, 2025 14:35
invariant.ts Outdated Show resolved Hide resolved
invariant.ts Outdated Show resolved Hide resolved
property.ts Outdated Show resolved Hide resolved
property.ts Outdated Show resolved Hide resolved
shared.ts Outdated Show resolved Hide resolved
shared.ts Outdated Show resolved Hide resolved
traits.ts Outdated Show resolved Hide resolved
traits.ts Outdated Show resolved Hide resolved
Copy link
Member

@moodmosaic moodmosaic 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 massive 🥇 After refactoring simnet and coming up with candidate alternatives for "enrich" this should be ready for review.

example/contracts/trait.clar Outdated Show resolved Hide resolved
ast1737030528990.json Outdated Show resolved Hide resolved
example/Clarinet.toml Show resolved Hide resolved
example/contracts/trait.tests.clar Outdated Show resolved Hide resolved
invariant.ts Outdated Show resolved Hide resolved
traits.tests.ts Outdated Show resolved Hide resolved
traits.tests.ts Outdated Show resolved Hide resolved
traits.tests.ts Outdated Show resolved Hide resolved
traits.tests.ts Show resolved Hide resolved
traits.ts Show resolved Hide resolved
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 restricts EnrichedContractInterfaceFunction type alias usage to
relevant methods and adds a JSDoc comment to the definition.
This commit refines the `traits` file by adding suggestive comments and fixing
types.
@BowTiedRadone BowTiedRadone changed the title [DRAFT] Select contracts implementing the correct trait from the Clarinet project for argument generation Select contracts implementing the correct trait from the Clarinet project for argument generation Jan 23, 2025
@BowTiedRadone BowTiedRadone marked this pull request as ready for review January 23, 2025 22:41
@BowTiedRadone BowTiedRadone requested a review from a team as a code owner January 23, 2025 22:41
@BowTiedRadone
Copy link
Collaborator Author

Gigantic cleaning session happened here. At least a couple things to decide on before merging:

  • Which contract that imports traits should be added to the example folder? It could be a handmade contract, but it should showcase the functionality clearly.
  • Which contract that implements traits do we want to use? This can either be a handmade contract added to the project or specified as a requirement.

Ideally, we should have two trait implementations for the same trait, so Rendezvous can randomly pick one of them for any trait reference parameter.

The bug was introduced in ae0d20c. This commit fixes it.
@moodmosaic
Copy link
Member

Which contract that imports traits should be added to the example folder? It could be a handmade contract, but it should showcase the functionality clearly.

Any contract that shows a real-world use case and stays under 100 lines.

Which contract that implements traits do we want to use? This can either be a handmade contract added to the project or specified as a requirement.

I'd say both: a local handmade one and one added as a requirement.

Copy link
Member

@moodmosaic moodmosaic 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 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)

citizen.tests.ts Show resolved Hide resolved
citizen.ts Show resolved Hide resolved
example/contracts/trait.clar Outdated Show resolved Hide resolved
invariant.ts Show resolved Hide resolved

/**
* The type of the function interface, after the contract interface is
* "enriched" with additional information about trait references.
Copy link
Member

Choose a reason for hiding this comment

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

I’ve thought about this and can’t find a better name. Enriched seems fine in this PR.

Why don't we expand the comment to help readers and contributors:

I would explain what a contract interface looks like, how the enriched version differs, and why. Use something similar to the comments in enrichInterfaceWithTraitData.

We could also point to tests like:
correctly enriches interface with trait reference data for a direct trait that is the first parameter.

I'd keep it simple and focused on readability for future maintainers or anyone continuing the project after us.

traits.types.ts Show resolved Hide resolved
traits.ts Show resolved Hide resolved
traits.ts Show resolved Hide resolved
traits.tests.ts 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.
@BowTiedRadone BowTiedRadone force-pushed the feat/project-trait-selection branch from 33432a1 to c76fa69 Compare January 24, 2025 18:26
[contracts.rendezvous-token]
path = 'contracts/rendezvous-token.clar'
clarity_version = 2
epoch = 2.5
Copy link
Member

Choose a reason for hiding this comment

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

To make it easier for future contributors, could we add a comment here explaining why this contract has epoch 2.5?

[contracts.trait]
path = 'contracts/trait.clar'
[contracts.ft-transfer-many]
path = 'contracts/ft-transfer-many.clar'
Copy link
Member

Choose a reason for hiding this comment

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

Some paths in this file use double quotes and some others use single quotes. Let's stick with one?

@@ -0,0 +1,21 @@
(use-trait ft-trait 'SP4SZE494VC2YC5JYG7AYFQ44F5Q4PYV7DVMDPBG.sip-010-trait-ft-standard.sip-010-trait)

(define-public (transfer-many
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(define-public (transfer-many
(define-public (send-tokens

🤔

(use-trait ft-trait 'SP4SZE494VC2YC5JYG7AYFQ44F5Q4PYV7DVMDPBG.sip-010-trait-ft-standard.sip-010-trait)

(define-public (transfer-many
(transfers-list
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(transfers-list
(transfers

🤔


(define-public (transfer-many
(transfers-list
(list 5 {token: <ft-trait>, recipient: principal, amount: uint})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(list 5 {token: <ft-trait>, recipient: principal, amount: uint})
(list 5 {token: <ft-trait>, to: principal, amount: uint})

🤔

(list 5 {token: <ft-trait>, recipient: principal, amount: uint})
)
)
(ok (map transfer transfers-list))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(ok (map transfer transfers-list))
(ok (map transfer transfers))

🤔

)

(define-private (transfer
(one-transfer {token: <ft-trait>, recipient: principal, amount: uint})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(one-transfer {token: <ft-trait>, recipient: principal, amount: uint})
(one-transfer {token: <ft-trait>, to: principal, amount: uint})

🤔

@@ -26,8 +34,8 @@ path = "contracts/slice.clar"
clarity_version = 3
epoch = 3.0

[contracts.trait]
path = 'contracts/trait.clar'
[contracts.ft-transfer-many]
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a more friendly name could be send-tokens? We have counter, cargo, etc. (Like, imagine if cargo was called creade-read-upate-delete.clar.) 👀

@@ -0,0 +1,47 @@
;; Invariants

(define-read-only (invariant-trait (address principal))
Copy link
Member

@moodmosaic moodmosaic Jan 24, 2025

Choose a reason for hiding this comment

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

Is the name invariant-trait left over from the previous contract (trait.clar)?

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.

Support for traits
2 participants