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

feat: add pox-4 signer-key to StackingClient methods #1614

Merged
merged 24 commits into from Mar 22, 2024

Conversation

janniks
Copy link
Collaborator

@janniks janniks commented Jan 3, 2024

This PR was published to npm with the version 6.12.2-pr.2a2f6aa.0
e.g. npm install @stacks/common@6.12.2-pr.2a2f6aa.0 --save-exact

  • pox-4 changes
    • adds signer-key to methods (as optional arguments, so this change can be merged before pox-4 is live)
    • removes deprecated data-vars

Waiting on

Copy link

vercel bot commented Jan 3, 2024

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

Name Status Preview Comments Updated (UTC)
stacksjs-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 21, 2024 4:46pm

Copy link

codecov bot commented Jan 4, 2024

Codecov Report

Attention: Patch coverage is 64.38356% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 66.31%. Comparing base (4ef1815) to head (2a2f6aa).

Files Patch % Lines
packages/stacking/src/index.ts 48.83% 17 Missing and 5 partials ⚠️
packages/stacking/src/utils.ts 86.66% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1614      +/-   ##
==========================================
- Coverage   66.39%   66.31%   -0.08%     
==========================================
  Files         119      119              
  Lines        8719     8764      +45     
  Branches     1947     1953       +6     
==========================================
+ Hits         5789     5812      +23     
- Misses       2692     2708      +16     
- Partials      238      244       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@janniks janniks force-pushed the feat/update-stacking-client-to-pox-4 branch from f09ba06 to 100c09a Compare January 4, 2024 16:43
@janniks janniks changed the title feat: add pox-4 signer-key to StackingClient.stack feat: add pox-4 signer-key to StackingClient methods Jan 4, 2024
@janniks janniks marked this pull request as ready for review January 4, 2024 18:57
@janniks janniks requested a review from zone117x January 4, 2024 18:57
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.

What happens if a signer key arg is provided before pox-4 is activated?

@janniks
Copy link
Collaborator Author

janniks commented Jan 5, 2024

Good point 😉 For now I assumed folks adding a signer-key know what they're doing, currently the ABI would fail.
But it's probably better to add another throw to the 'ensure' helper for both missing and given signer-key and the respective pox.

@janniks
Copy link
Collaborator Author

janniks commented Jan 8, 2024

Added a better ensure check.
Also waiting to see if stacks-network/stacks-core#4230 is planned.
The API should be able to use the pre-release of this PR

@friedger
Copy link
Collaborator

Delegate stack stx won't have signer key. There is an issue stacks-network/stacks-core#4274

@hstove
Copy link
Contributor

hstove commented Jan 24, 2024

Also FYI new function arg signer-sig for a few functions: stacks-network/stacks-core#4277

@janniks
Copy link
Collaborator Author

janniks commented Jan 30, 2024

Thanks 🙏 Will wait for the PRs to merge to update the remainder

@hstove
Copy link
Contributor

hstove commented Feb 8, 2024

Added a helper for the signer signature: #1628

Now that this is in the next branch, I don't think there will be any more changes to the pox-4 function signatures.

@janniks
Copy link
Collaborator Author

janniks commented Mar 21, 2024

Changed some minor missing checks/params and addressed review comments. Could I get a re-review on this? @zone117x or @hstove

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.

LGTM, nice job @janniks!

@hstove
Copy link
Contributor

hstove commented Mar 21, 2024

This is great, and I've been testing with a local version of Lockstacks and it works as expected.

One change request, though: can you change authId to be IntegerType? We could run into issues with the number type because it can technically be any u128.

@janniks
Copy link
Collaborator Author

janniks commented Mar 21, 2024

Done ✅ Thanks

@janniks janniks merged commit 3e649de into main Mar 22, 2024
9 of 10 checks passed
@janniks janniks deleted the feat/update-stacking-client-to-pox-4 branch March 22, 2024 13:56
@janniks janniks restored the feat/update-stacking-client-to-pox-4 branch March 22, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants