Shd/governance validation2 #457
Conversation
…github.com:input-output-hk/acropolis into shd/governance-validation
sandtreader
left a comment
sandtreader
left a comment
There was a problem hiding this comment.
This is a great pattern to do this error promotion across the board - thanks!
Some comments on naming - nothing substantive so go ahead and merge if it's holding anything up.
| } | ||
|
|
||
| pub fn conway() -> Self { | ||
| Self { major: 9, minor: 0 } |
There was a problem hiding this comment.
This could be confusing - Conway is (at least) protocol versions 9 (Chang) and 10 (Plomin) and could well include 11 soon. It would be better to call this chang() I think.
| macro_rules! declare_cardano_reader { | ||
| ($name:ident, $msg_constructor:ident, $msg_type:ty) => { | ||
| async fn $name(s: &mut Box<dyn Subscription<Message>>) -> Result<(BlockInfo, $msg_type)> { | ||
| info!("Waiting in topic {}", stringify!($msg_constructor)); |
There was a problem hiding this comment.
Unlikely we'd want this in production :-)
There was a problem hiding this comment.
There's a standing joke about files called 'utils' ;-) The validation bits could move to validation.rs, maybe the remaining macro in messaging.rs or some such?
| /// See Haskell Node, Cardano.Ledger.BaseTypes: Cardano/Src/Ledger/BaseTypes.hs | ||
| #[derive(Clone, Debug, serde::Serialize, serde::Deserialize, PartialEq, Eq)] | ||
| pub enum MismatchRelation { | ||
| RelEq, |
There was a problem hiding this comment.
Style point - is the Rel prefix needed given it's always qualified with MismatchRelation anyway?
| /// See Haskell Node, Cardano.Ledger.BaseTypes: Cardano/Src/Ledger/BaseTypes.hs | ||
| #[derive(Clone, Debug, serde::Serialize, serde::Deserialize, PartialEq, Eq)] | ||
| pub enum Mismatch<T: Debug + Display> { | ||
| Supplied(T, MismatchRelation), |
There was a problem hiding this comment.
Why does Supplied have a relation too? I'm assuming we're representing "we got X but expected REL Y".
Also confused why it's an enum - maybe naming is strange? Let's not import Haskell weirdness if we can avoid it!
| try_join_all(validator_topics.iter().map(|topic| context.subscribe(topic))).await?; | ||
|
|
||
| // True if we expect validation to be performed by the nodes | ||
| let do_validation = !validator_subscriptions.is_empty(); |
| let prev = self.proposals.insert(proc.gov_action_id.clone(), (epoch, proc.clone())); | ||
| if let Some(prev) = prev { | ||
| return Err(anyhow!( | ||
| outcomes.push_anyhow(anyhow!( |
There was a problem hiding this comment.
Is there no specific governance error we could generate here? It seems like you have done in most cases.
| let (blk_p, params) = Self::read_parameters(&mut protocol_s).await?; | ||
| if blk_g != blk_p { | ||
| error!( | ||
| outcomes.push_anyhow(anyhow!( |
There was a problem hiding this comment.
Interesting... I hadn't thought of promoting this kind of internal error, but it is the safe thing to do - nothing can be valid if we're screwed up internally.
| (Conway "GOV" rule) | ||
|
|
||
| ``` | ||
| data ConwayGovPredFailure era |
There was a problem hiding this comment.
Looks like there are a lot more errors defined here than we can currently generate - need a ticket to add checks on these (in time).
golddydev
left a comment
golddydev
left a comment
There was a problem hiding this comment.
ValidationOutcome approach is nice 👍🏼
Willl use that with UTxO validation
Description
Related Issue(s)
Fixes #[#404] (#404)
How was this tested?
Checklist
Impact / Side effects
Reviewer notes / Areas to focus
Added common/utils.rs for validation help.