-
Notifications
You must be signed in to change notification settings - Fork 15
serviceability: add ip_verifier_authority_pk to GlobalState and rotate it via SetAuthority #4207
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,8 @@ pub struct AuthorityDisplay { | |
| pub feed_authority: Pubkey, | ||
| #[serde(serialize_with = "serializer::serialize_pubkey_as_string")] | ||
| pub health_oracle: Pubkey, | ||
| #[serde(serialize_with = "serializer::serialize_pubkey_as_string")] | ||
| pub ip_verifier_authority: Pubkey, | ||
| } | ||
|
|
||
| impl GetAuthorityCliCommand { | ||
|
|
@@ -41,6 +43,7 @@ impl GetAuthorityCliCommand { | |
| access_authority: gstate.sentinel_authority_pk, | ||
| feed_authority: gstate.feed_authority_pk, | ||
| health_oracle: gstate.health_oracle_pk, | ||
| ip_verifier_authority: gstate.ip_verifier_authority_pk, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An unset verifier renders as the literal Concrete case: right after the program upgrade but before the first rotation, an operator runs Rendering |
||
| }; | ||
|
|
||
| if self.json { | ||
|
|
@@ -79,6 +82,7 @@ mod tests { | |
| let sentinel_authority = Pubkey::new_unique(); | ||
| let feed_authority = Pubkey::new_unique(); | ||
| let health_oracle = Pubkey::new_unique(); | ||
| let ip_verifier_authority = Pubkey::new_unique(); | ||
| let globalstate = GlobalState { | ||
| account_type: AccountType::GlobalState, | ||
| bump_seed: 0, | ||
|
|
@@ -94,6 +98,7 @@ mod tests { | |
| qa_allowlist: vec![], | ||
| feature_flags: 0, | ||
| feed_authority_pk: feed_authority, | ||
| ip_verifier_authority_pk: ip_verifier_authority, | ||
| }; | ||
|
|
||
| client | ||
|
|
@@ -129,6 +134,10 @@ mod tests { | |
| has_row("health_oracle", &health_oracle.to_string()), | ||
| "health_oracle row should contain value" | ||
| ); | ||
| assert!( | ||
| has_row("ip_verifier_authority", &ip_verifier_authority.to_string()), | ||
| "ip_verifier_authority row should contain value" | ||
| ); | ||
|
|
||
| // JSON output | ||
| let mut output = Vec::new(); | ||
|
|
@@ -153,5 +162,9 @@ mod tests { | |
| json["health_oracle"].as_str().unwrap(), | ||
| health_oracle.to_string() | ||
| ); | ||
| assert_eq!( | ||
| json["ip_verifier_authority"].as_str().unwrap(), | ||
| ip_verifier_authority.to_string() | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -498,6 +498,7 @@ mod tests { | |
| fn gs_with_feed(authority: &Pubkey) -> GlobalState { | ||
| GlobalState { | ||
| feed_authority_pk: *authority, | ||
| ip_verifier_authority_pk: Pubkey::default(), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: redundant next to |
||
| ..GlobalState::default() | ||
| } | ||
| } | ||
|
|
@@ -523,6 +524,7 @@ mod tests { | |
| sentinel_authority_pk: sentinel, | ||
| health_oracle_pk: health_oracle, | ||
| feed_authority_pk: feed, | ||
| ip_verifier_authority_pk: Pubkey::default(), | ||
| ..GlobalState::default() | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,14 +21,19 @@ pub struct SetAuthorityArgs { | |
| pub sentinel_authority_pk: Option<Pubkey>, | ||
| pub health_oracle_pk: Option<Pubkey>, | ||
| pub feed_authority_pk: Option<Pubkey>, | ||
| pub ip_verifier_authority_pk: Option<Pubkey>, | ||
| } | ||
|
|
||
| impl fmt::Debug for SetAuthorityArgs { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| write!( | ||
| f, | ||
| "activator_authority_pk: {:?}, sentinel_authority_pk: {:?}, health_oracle_pk: {:?}, feed_authority_pk: {:?}", | ||
| self.activator_authority_pk, self.sentinel_authority_pk, self.health_oracle_pk, self.feed_authority_pk | ||
| "activator_authority_pk: {:?}, sentinel_authority_pk: {:?}, health_oracle_pk: {:?}, feed_authority_pk: {:?}, ip_verifier_authority_pk: {:?}", | ||
| self.activator_authority_pk, | ||
| self.sentinel_authority_pk, | ||
| self.health_oracle_pk, | ||
| self.feed_authority_pk, | ||
| self.ip_verifier_authority_pk | ||
| ) | ||
| } | ||
| } | ||
|
|
@@ -95,6 +100,9 @@ pub fn process_set_authority( | |
| if let Some(feed_authority_pk) = value.feed_authority_pk { | ||
| globalstate.feed_authority_pk = feed_authority_pk; | ||
| } | ||
| if let Some(ip_verifier_authority_pk) = value.ip_verifier_authority_pk { | ||
| globalstate.ip_verifier_authority_pk = ip_verifier_authority_pk; | ||
| } | ||
|
Comment on lines
+103
to
+105
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The field's own doc comment defines Non-blocking, but the guard is cheap and matches the semantics the field already documents: if let Some(pk) = value.ip_verifier_authority_pk {
if pk == Pubkey::default() {
return Err(DoubleZeroError::InvalidAccountData.into()); // or the closest existing variant
}
globalstate.ip_verifier_authority_pk = pk;
}I'd only guard the new field — retrofitting the other four is a behavior change that doesn't belong in this PR. |
||
|
|
||
| try_acc_write(&globalstate, globalstate_account, payer_account, accounts)?; | ||
|
|
||
|
|
@@ -103,3 +111,37 @@ pub fn process_set_authority( | |
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| /// Transactions built before `ip_verifier_authority_pk` existed encode only the | ||
| /// first four options. `BorshDeserializeIncremental` must still decode them, | ||
| /// leaving the new field as `None` (i.e. "leave it alone"). | ||
| #[test] | ||
| fn test_setauthority_args_decodes_pre_ip_verifier_encoding() { | ||
| let feed = Pubkey::new_unique(); | ||
|
|
||
| let mut data = Vec::new(); | ||
| None::<Pubkey>.serialize(&mut data).unwrap(); | ||
| None::<Pubkey>.serialize(&mut data).unwrap(); | ||
| None::<Pubkey>.serialize(&mut data).unwrap(); | ||
| Some(feed).serialize(&mut data).unwrap(); | ||
|
|
||
| let args = SetAuthorityArgs::try_from(&data[..]).unwrap(); | ||
| assert_eq!(args.feed_authority_pk, Some(feed)); | ||
| assert_eq!(args.ip_verifier_authority_pk, None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_setauthority_args_roundtrips_ip_verifier() { | ||
| let args = SetAuthorityArgs { | ||
| ip_verifier_authority_pk: Some(Pubkey::new_unique()), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let data = borsh::to_vec(&args).unwrap(); | ||
| assert_eq!(SetAuthorityArgs::try_from(&data[..]).unwrap(), args); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This golden now advertises
IpVerifierAuthorityPk, but nothing validates it:GlobalStatestruct (smartcontract/sdk/go/serviceability/state.go:74-88) stops atFeedAuthorityPK, andfixture_test.gohas no GlobalState case at all.fixtures.test.ts:if (!(f.name in got)) continue;) and Python does the same (test_fixtures.py:if name not in got: continue).So these bytes can drift or regress undetected. I know the SDK deserializer work is tracked separately — the point here is just that the golden currently has zero coverage for the field it advertises, which is a slightly worse state than not adding it yet. Adding
IpVerifierAuthorityPKto the Go struct plus a GlobalState fixture case would close it.