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 some more subnet contract tests #227

Merged
merged 11 commits into from
Mar 15, 2023

Conversation

jbencin
Copy link
Member

@jbencin jbencin commented Mar 6, 2023

Description

Added some failing tests for the subnet.clar contract. Some examples include:

  • Try to register miner twice
  • Try to withdraw using invalid root/leaf/sibling hashes
  • Try to withdraw to wrong principal
  • Try to overflow token balance

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@jbencin jbencin requested a review from obycode March 6, 2023 22:32
@codecov-commenter
Copy link

codecov-commenter commented Mar 6, 2023

Codecov Report

Merging #227 (409b9be) into master (e23b941) will increase coverage by 93.47%.
The diff coverage is 100.00%.

❗ Current head 409b9be differs from pull request most recent head 503844b. Consider uploading reports for the commit 503844b to get more accurate results

@@             Coverage Diff             @@
##           master     #227       +/-   ##
===========================================
+ Coverage        0   93.47%   +93.47%     
===========================================
  Files           0        6        +6     
  Lines           0      337      +337     
===========================================
+ Hits            0      315      +315     
- Misses          0       22       +22     
Impacted Files Coverage Δ
core-contracts/contracts/helper/simple-ft.clar 76.47% <100.00%> (ø)

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@obycode obycode left a comment

Choose a reason for hiding this comment

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

Before I go through all of the details in subnet_test.ts, could you separate out those new assertions into separate tests, so each one is a separate Clarinet.test? This makes quickly reviewing the results of the test much simpler.

@@ -0,0 +1,21 @@
# Subnets Core Contracts

Essential Clarity contracts for using Stacks Subnets
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of putting a README in here. Let's expand it a bit.

Suggested change
Essential Clarity contracts for using Stacks Subnets
This directory contains the contracts published to the Stacks L1 to implement a subnet.
* _subnet.clar_: interface between the subnet and the L1
* _multi-miner.clar_: implements a multi-miner for the subnet
* _helper/*_: used for testing

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure whether to do this or not because it takes a decent amount of setup to get to the point where I can attempt these transactions, but yeah I can make this change

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think it's worth it, since these tests don't take long to run.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was the code duplication that I wanted to avoid. Anyway, I did split most of my new tests out into separate tests. Also fixed all the remaining deno lint warnings in the subnet_test.ts file

@jbencin jbencin force-pushed the test/add-subnet-contract-tests branch from 1664a5a to 409b9be Compare March 14, 2023 19:40
@obycode
Copy link
Member

obycode commented Mar 15, 2023

Go ahead and remove the draft status on this when you're ready for a review. Is it ready now?

Copy link
Member

@obycode obycode 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 great. Just one minor comment.

@jbencin jbencin marked this pull request as ready for review March 15, 2023 18:53
@jbencin jbencin merged commit 7012d22 into hirosystems:master Mar 15, 2023
@jbencin jbencin deleted the test/add-subnet-contract-tests branch April 18, 2023 19:13
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.

Test more error cases
3 participants