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]: print signer version and pubkey on startup #1279

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MCJOHN974
Copy link
Collaborator

@MCJOHN974 MCJOHN974 commented Jan 28, 2025

Description

This PR changes signer binary such that it prints signer version and pubkey to logs on startup

Closes: #1106

Changes

Added few log lines

Testing Information

I think testing here is not worth the effort, so just tried to run this locally

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@MCJOHN974 MCJOHN974 requested a review from djordon January 28, 2025 12:06
@MCJOHN974 MCJOHN974 marked this pull request as ready for review January 28, 2025 13:31
"starting the sBTC signer",
);

// Load the configuration file and/or environment variables.
let settings = Settings::new(args.config)?;

tracing::info!(
signer_public_key = %PublicKey::from_private_key(&settings.signer.private_key),
Copy link
Member

@cylewitruk cylewitruk Jan 28, 2025

Choose a reason for hiding this comment

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

I think I would just add this to the above startup log fields tbh, we don't really need to explicitly know that the config has been loaded successfully because it will exit with an error if it fails.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Than we want to create settings before writing that first log line. If you think it will be better I don't mind we can do it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having the above log before the config creation makes sense to me -- we are sure we get some logs in case there were bad issues with the config itself.

I'm fine with having this log on a different logline, but I would:

  • add a method in SignerConfig returning the public key, and call it here
  • change the message from constructed signer settings to something more tailored to the public key

@@ -74,11 +75,18 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
revision = signer::GIT_COMMIT,
arch = signer::TARGET_ARCH,
env_abi = signer::TARGET_ENV_ABI,
signer_version = signer::VERSION,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we are actively using the package version (I guess revision is already enough)

"starting the sBTC signer",
);

// Load the configuration file and/or environment variables.
let settings = Settings::new(args.config)?;

tracing::info!(
signer_public_key = %PublicKey::from_private_key(&settings.signer.private_key),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having the above log before the config creation makes sense to me -- we are sure we get some logs in case there were bad issues with the config itself.

I'm fine with having this log on a different logline, but I would:

  • add a method in SignerConfig returning the public key, and call it here
  • change the message from constructed signer settings to something more tailored to the public key

@djordon djordon added this to the sBTC: Deposits milestone Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

[Feature]: Print version and public key on startup
4 participants