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 signer id use in stackerdb #4451

Merged
merged 2 commits into from Mar 1, 2024
Merged

Conversation

jferrant
Copy link
Collaborator

@jferrant jferrant commented Feb 29, 2024

In my massive PR, I aaccidentally conflated signer id and signer slot id. this did not break tests because we happened to have everything line up (odds are we ALWAYS will have everything line up since the reward set is used to buidl stackerdb, but this will make sure we do not rely on this)

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

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

Project coverage is 77.52%. Comparing base (911c3ff) to head (73fd5e0).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4451      +/-   ##
==========================================
- Coverage   83.46%   77.52%   -5.94%     
==========================================
  Files         448      448              
  Lines      324308   324321      +13     
==========================================
- Hits       270676   251424   -19252     
- Misses      53632    72897   +19265     
Files Coverage Δ
stacks-signer/src/client/mod.rs 99.09% <100.00%> (ø)
stacks-signer/src/client/stackerdb.rs 91.56% <100.00%> (-0.08%) ⬇️
stacks-signer/src/client/stacks_client.rs 83.03% <100.00%> (-3.71%) ⬇️
stacks-signer/src/config.rs 84.41% <ø> (ø)
stacks-signer/src/coordinator.rs 76.35% <100.00%> (ø)
stacks-signer/src/runloop.rs 90.50% <96.72%> (+2.36%) ⬆️
testnet/stacks-node/src/tests/signer.rs 66.98% <0.00%> (-26.41%) ⬇️
stacks-signer/src/signer.rs 73.66% <72.72%> (-0.14%) ⬇️

... and 173 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 911c3ff...73fd5e0. Read the comment docs.

xoloki
xoloki previously approved these changes Feb 29, 2024
Copy link
Collaborator

@xoloki xoloki left a comment

Choose a reason for hiding this comment

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

LGTM, one small question about a comment.

stacks-signer/src/client/mod.rs Outdated Show resolved Hide resolved
stacks-signer/src/client/mod.rs Outdated Show resolved Hide resolved
hstove
hstove previously approved these changes Feb 29, 2024
kantai
kantai previously approved these changes Feb 29, 2024
@jferrant jferrant force-pushed the feat/allow-single-transaction-per-signer branch 2 times, most recently from b93c1e9 to c533146 Compare March 1, 2024 14:34
@jferrant jferrant changed the base branch from feat/allow-single-transaction-per-signer to next March 1, 2024 18:12
@jferrant jferrant dismissed stale reviews from kantai, hstove, and xoloki March 1, 2024 18:12

The base branch was changed.

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant force-pushed the bugfix/misuse-signer-id-signer-slot-id branch from 3208ed2 to 73fd5e0 Compare March 1, 2024 18:13
@jferrant jferrant enabled auto-merge March 1, 2024 18:37
@jferrant jferrant added this pull request to the merge queue Mar 1, 2024
@jferrant jferrant removed this pull request from the merge queue due to a manual request Mar 1, 2024
@jferrant jferrant enabled auto-merge March 1, 2024 20:02
@jferrant jferrant added this pull request to the merge queue Mar 1, 2024
Merged via the queue into next with commit 4888ba2 Mar 1, 2024
1 of 2 checks passed
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.

None yet

4 participants