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

update signer logging to include reward cycle to differentiate curren… #4505

Merged
merged 2 commits into from Mar 8, 2024

Conversation

jferrant
Copy link
Collaborator

@jferrant jferrant commented Mar 7, 2024

added reward cycle to signer logs to differentiate between current and next reward cycle signer state machines

hstove
hstove previously approved these changes Mar 7, 2024
Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

Approving because I think it's better to merge now, but I think the size of the diff here demonstrates that it would be helpful to have some kind of log_label(signer) helper. For example, it might also be prudent to log the pubkey or address

But that's a nit, this LGTM

Copy link

codecov bot commented Mar 7, 2024

Codecov Report

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

Project coverage is 82.44%. Comparing base (078a984) to head (28bcd1b).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4505      +/-   ##
==========================================
- Coverage   83.26%   82.44%   -0.83%     
==========================================
  Files         452      452              
  Lines      326008   325964      -44     
  Branches      323      323              
==========================================
- Hits       271444   268730    -2714     
- Misses      54556    57226    +2670     
  Partials        8        8              
Files Coverage Δ
stacks-signer/src/runloop.rs 90.20% <72.72%> (-0.68%) ⬇️
stacks-signer/src/signer.rs 77.13% <50.45%> (+3.37%) ⬆️

... and 36 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 078a984...28bcd1b. Read the comment docs.

xoloki
xoloki previously approved these changes Mar 7, 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

@jferrant jferrant requested a review from hstove March 7, 2024 21:20
obycode
obycode previously approved these changes Mar 8, 2024
@jferrant
Copy link
Collaborator Author

jferrant commented Mar 8, 2024

Approving because I think it's better to merge now, but I think the size of the diff here demonstrates that it would be helpful to have some kind of log_label(signer) helper. For example, it might also be prudent to log the pubkey or address

But that's a nit, this LGTM

Yes I considered this but assumed could be done later. Just did a quick search replace... But will do now as its also basically a quick search replace XD

…t and next signers

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant dismissed stale reviews from obycode, xoloki, and hstove via 3022282 March 8, 2024 14:54
@jferrant jferrant force-pushed the chore/print-signer-reward-cycle-in-all-logs branch from de9494b to 3022282 Compare March 8, 2024 14:54
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant force-pushed the chore/print-signer-reward-cycle-in-all-logs branch from 3022282 to 28bcd1b Compare March 8, 2024 14:57
@jferrant jferrant requested review from xoloki and obycode March 8, 2024 14:59
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

Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

LGTM!

@jferrant jferrant enabled auto-merge March 8, 2024 16:11
@jferrant jferrant added this pull request to the merge queue Mar 8, 2024
Merged via the queue into next with commit 3b6b05c Mar 8, 2024
2 checks passed
@hstove hstove deleted the chore/print-signer-reward-cycle-in-all-logs branch March 8, 2024 16:56
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