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

Use new local-signer package #716

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

richardpringle
Copy link
Contributor

Why this should be merged

I've moved specific signing implementations outside of the bls package

How this was tested

No functional changes, so just unit tests

Need to be documented?

Nope

Need to update RELEASES.md?

Nope

@richardpringle richardpringle requested review from ceyonur, darioush and a team as code owners December 20, 2024 18:20
utils/snow.go Outdated
@@ -13,7 +13,7 @@ import (
"github.com/ava-labs/avalanchego/snow/validators"
"github.com/ava-labs/avalanchego/snow/validators/validatorstest"
"github.com/ava-labs/avalanchego/utils/constants"
"github.com/ava-labs/avalanchego/utils/crypto/bls"
"github.com/ava-labs/avalanchego/utils/crypto/bls/signers/local"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I rename this to localSigner or something similar?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Preferrably localsigner all in lowercase

qdm12
qdm12 previously approved these changes Dec 30, 2024
@qdm12
Copy link
Collaborator

qdm12 commented Dec 30, 2024

Tests need to be updated for imports: https://github.com/ava-labs/coreth/actions/runs/12436458610/job/34724339849?pr=716 🙏

go.mod Show resolved Hide resolved
Copy link
Collaborator

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

All good except let's keep the short commit hash to 12 characters as it was to avoid any surprise for now.

@@ -6,4 +6,4 @@
set -euo pipefail

# Don't export them as they're used in the context of other calls
AVALANCHE_VERSION=${AVALANCHE_VERSION:-'902377d447ba'}
AVALANCHE_VERSION=${AVALANCHE_VERSION:-'8885a5c44'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the short commit hash here shorter than the previous existing one? (12 vs 9 characters)

Let's keep it as it was ideally?

Copy link
Collaborator

@darioush darioush Jan 7, 2025

Choose a reason for hiding this comment

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

It needs to be 8 bytes in subnet-evm to match the avalanchego docker tag. So let's keep it 8 here as well.

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.

4 participants