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

fix: remove stacking into period3 check #1402

Merged
merged 2 commits into from
Nov 30, 2022

Conversation

janniks
Copy link
Collaborator

@janniks janniks commented Nov 30, 2022

This PR was published to npm with the version 6.0.2-pr.da30fce.0
e.g. npm install @stacks/[email protected] --save-exact

This bug was a remnant of not understanding the periods initially.

I believe, the check to see if something overlaps into Period 3 is no longer needed (and was implemented incorrectly before).
It's not needed since Stacks.js will prefer PoX-2 if available, so @stacks/stacking shouldn't be able to generate txs that target PoX-1 after the 2.1 fork.

fix:

  • removes ensurePox1DoesNotCreateStateIntoPeriod3 check

test:

  • adds more extensive tests for correct contracts usage

I will deprecate the faulty version on npm after releasing this fix.

@janniks janniks added the P1 label Nov 30, 2022
@janniks janniks requested a review from zone117x November 30, 2022 15:45
@janniks janniks self-assigned this Nov 30, 2022
@vercel
Copy link

vercel bot commented Nov 30, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
stacksjs-docs ✅ Ready (Inspect) Visit Preview Nov 30, 2022 at 3:45PM (UTC)

@codecov-commenter
Copy link

Codecov Report

Merging #1402 (da30fce) into master (ee52662) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1402      +/-   ##
==========================================
- Coverage   63.52%   63.49%   -0.04%     
==========================================
  Files         116      116              
  Lines        8508     8500       -8     
  Branches     1879     1875       -4     
==========================================
- Hits         5405     5397       -8     
  Misses       2974     2974              
  Partials      129      129              
Impacted Files Coverage Δ
packages/stacking/src/index.ts 90.27% <ø> (-0.07%) ⬇️
packages/stacking/src/utils.ts 61.93% <ø> (-2.05%) ⬇️
packages/stacking/tests/apiMockingHelpers.ts 48.10% <100.00%> (+2.76%) ⬆️

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

@zone117x zone117x left a comment

Choose a reason for hiding this comment

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

To clarify my understanding, this change ensures "if stacking and if PoX-2 contract is deployed, then submit tx for the pox-2 contract", right?

@janniks
Copy link
Collaborator Author

janniks commented Nov 30, 2022

To clarify my understanding, this change ensures "if stacking and if PoX-2 contract is deployed, then submit tx for the pox-2 contract", right?

Essentially, the behavior already previously was: IF now == Period1 THEN use pox-1 ELSE use pox-2, where Period1 = beforeFork || isPox2NotYetConfigured. This behavior is unchanged by this PR.


This PR just removes the ensurePox1DoesNotCreateStateIntoPeriod3 check.

This check was buggy, since it didn't check the current contract at all. And the check doesn't make sense now anyway — since non-Period1 will always use pox-2. So, either overlapping-into-Period3 would be allowed (in Period1 via pox-1) or we use pox-2 anyway.

@janniks janniks merged commit d38fcc2 into master Nov 30, 2022
@janniks janniks deleted the fix/remove-stacking-into-period3-check branch November 30, 2022 16:57
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.

3 participants