Closed
Conversation
GTC6244
had a problem deploying
to
project-manager
GitHub Actions
Failure
GTC6244
had a problem deploying
to
naga-staging
GitHub Actions
Failure
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds threshold enforcement to keysets. The changes refactor the root key handling from a HashMap-based approach to a Vec-based approach, simplifying the key share proof checking logic and removing per-keyset iteration in favor of a single validation pass.
- Refactored root key storage from
HashMap<String, Vec<CachedRootKey>>toVec<CachedRootKey> - Updated voting and validation logic to work with the simplified structure
- Added new error handling for threshold enforcement
- Updated contract ABIs to support new error types
Reviewed Changes
Copilot reviewed 40 out of 73 changed files in this pull request and generated 45 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/lit-node/lit-node/src/tasks/fsm/utils.rs | Refactored key share proof checking from keyset-based to single pass validation |
| rust/lit-node/lit-node/src/tasks/fsm/mod.rs | Updated root key handling to use Vec instead of HashMap |
| rust/lit-node/lit-node/src/tasks/fsm/epoch_change.rs | Changed return type to Result and improved error handling |
| rust/lit-node/lit-node/src/tss/ecdsa_damfast/mod.rs | Made root_pubkeys parameter optional |
| rust/lit-core/lit-blockchain/abis/Staking.json | Added CannotKickBelowKeySetThreshold error and reordered NotEnoughValidatorsInNextEpoch |
| rust/lit-core/Cargo.toml | Pinned scc dependency to specific version 3.3.2 |
| rust/lit-node/lit-node/tests/integration/backup_long.rs | Updated imports for elliptic_curve crate |
| rust/lit-node/lit-node/tests/component/sign/ecdsa_damfast.rs | Updated function call to pass Option for root keys |
| rust/lit-core/lit-blockchain/src/contracts/ | Updated contract bindings with new bytecode |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .map_err(|e| unexpected_err(e, Some("Unable to complain".to_string())))?; | ||
| } else { | ||
| error!( | ||
| "Key share proof verification failed for peer {} - curve {}: {:?} - already complainted for this DKG.", |
There was a problem hiding this comment.
Typo: "complainted" should be "complained".
| import * as fs from 'fs'; | ||
| import * as path from 'path'; | ||
| import * as crypto from 'crypto'; | ||
| import { env } from 'process'; |
There was a problem hiding this comment.
Unused import env.
Suggested change
| import { env } from 'process'; |
| } | ||
| ); | ||
|
|
||
| const schedulerJob = new gcp.cloudscheduler.Job( |
There was a problem hiding this comment.
Unused variable schedulerJob.
Suggested change
| const schedulerJob = new gcp.cloudscheduler.Job( | |
| new gcp.cloudscheduler.Job( |
| ); | ||
|
|
||
| // Create the escalations. | ||
| const declareIncident = new grafana.oncall.Escalation( |
There was a problem hiding this comment.
Unused variable declareIncident.
Suggested change
| const declareIncident = new grafana.oncall.Escalation( | |
| new grafana.oncall.Escalation( |
|
|
||
| function setupPermissionsForCloudRunJob(cloudRunServiceAccount: Account) { | ||
| // Grant the Cloud Run Job's service account the role to publish metrics | ||
| const metricWriterRole = new gcp.projects.IAMBinding( |
There was a problem hiding this comment.
Unused variable metricWriterRole.
Suggested change
| const metricWriterRole = new gcp.projects.IAMBinding( | |
| new gcp.projects.IAMBinding( |
| } | ||
| ); | ||
|
|
||
| const sev4OrLessRoute = new grafana.oncall.Route( |
There was a problem hiding this comment.
Unused variable sev4OrLessRoute.
| // We only have one route for the Sentry webhook integration since Sentry does not distinguish | ||
| // between different severity levels. | ||
| // NOTE: We do NOT have a sev1 route at this moment. | ||
| const sev4RouteSentry = new grafana.oncall.Route( |
There was a problem hiding this comment.
Unused variable sev4RouteSentry.
Suggested change
| const sev4RouteSentry = new grafana.oncall.Route( | |
| new grafana.oncall.Route( |
| ); | ||
|
|
||
| // Create routes for the Zendesk webhook integration. | ||
| const infraSev2Route = new grafana.oncall.Route( |
There was a problem hiding this comment.
Unused variable infraSev2Route.
Suggested change
| const infraSev2Route = new grafana.oncall.Route( | |
| new grafana.oncall.Route( |
| } | ||
| ); | ||
|
|
||
| const infraSev3Route = new grafana.oncall.Route( |
There was a problem hiding this comment.
Unused variable infraSev3Route.
| } | ||
| ); | ||
|
|
||
| const infraSev4OrLessRoute = new grafana.oncall.Route( |
There was a problem hiding this comment.
Unused variable infraSev4OrLessRoute.
Suggested change
| const infraSev4OrLessRoute = new grafana.oncall.Route( | |
| new grafana.oncall.Route( |
Contributor
Author
|
Superseded by #33 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHAT
Add threshold enforcement to keysets
Superseded by #33