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 puzzles, driver and lifecycle tests for revocable CATs #19000

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

greimela
Copy link
Contributor

@greimela greimela commented Dec 9, 2024

Details can be found in the accompanying CHIP.

Details can be found in the accompanying CHIP
@greimela greimela requested a review from a team as a code owner December 9, 2024 09:15
Copy link
Contributor

@Quexington Quexington left a comment

Choose a reason for hiding this comment

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

Happy to let this sit in PR for the sake of developing within chia-blockchain but I don't think this should be merged unless it's actually going to be used because otherwise it's clutter.

chia/wallet/revocable_cats/p2_delegated_by_singleton.clsp Outdated Show resolved Hide resolved
@@ -0,0 +1,64 @@
; This layer ensures that a second puzzle hash or "backdoor" that can be used without the inner puzzle's permission
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you duplicating this puzzle rather than importing the existing one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Various people didn't want to use the previous name.
So I've added it with a different name, since I didn't want to change the VC code.

def construct_p2_delegated_by_singleton(issuer_launcher_id: bytes32) -> Program:
singleton_struct: Program = Program.to(
(
SINGLETON_TOP_LAYER_MOD_HASH,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's important in these drivers to have optional ways to override these values. Sometimes, in tests, I like to use a "mock" singleton puzzle, and these drivers wouldn't work for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same goes for similar functions below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea, although as you said, I don't think that driver will be merged into chia-blockchain anytime soon.
Would add that in case we do want to use it here.

chia/_tests/wallet/test_revcat_lifecycle.py Outdated Show resolved Hide resolved


@pytest.mark.anyio
async def test_revocable_cat_lifecycle(cost_logger):
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this is copied from a real in-"production" test but it's from like 3 years ago. Using the standard puzzle as the p2 puzzle really over complicates the whole file and makes it difficult to review and get accurate cost metrics. I would rather use Program.to(1) as the inner puzzle where possible because it's the minimal-est puzzle that allows for what you need and therefore weighs on the cost number the least.

Copy link
Contributor Author

@greimela greimela Dec 11, 2024

Choose a reason for hiding this comment

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

That makes sense, will use that!
I've seen that with other tests, but my first thought was that using the "real" puzzle brings the tests closer to reality.
But that doesn't really matter, I imagine.


@pytest.mark.anyio
async def test_revocable_cat_lifecycle(cost_logger):
async with sim_and_client() as (sim, sim_client):
Copy link
Contributor

Choose a reason for hiding this comment

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

I left comments on the other file that should apparently be deleted about simplifying this test function.

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, that other file was a duplicate I've committed by mistake

@ChiaAutomation
Copy link
Contributor

Your commits are not signed and our branch protection rules require signed commits. For more information on how to create signed commits, please visit this page: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification. Please use the button towards the bottom of the page to close this pull request and open a new one with signed commits.

@emlowe emlowe added the Added Required label for PR that categorizes merge commit message as "Added" for changelog label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added Required label for PR that categorizes merge commit message as "Added" for changelog community-pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants