Conversation
📝 WalkthroughWalkthroughAdds a new governance event model in event/governance.go with exported types: GovernanceContext, GovernanceEvent, ProposalProcedureData, VotingProcedureData, DRepCertificateData, VoteDelegationCertificateData, CommitteeCertificateData, and AnchorData. Adds constructors NewGovernanceContext and NewGovernanceEvent and utility HasGovernanceData. GovernanceEvent supports optional CBOR inclusion and serializes hash/anchor fields as JSON-friendly hex. chainsync.go now emits a governance event for transactions containing governance data in handleRollForward and handleBlockFetchBlock, using the block, transaction, index, networkMagic, and includeCbor parameters. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)input/chainsync/chainsync.go (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @event/governance.go:
- Around line 198-202: The code accesses prop.Anchor() directly and assigns
anchor.Url and anchor.DataHash to data.Anchor which can panic if anchor is nil;
modify the block around anchor := prop.Anchor() and the assignment to
data.Anchor so you first check if anchor != nil and only then set data.Anchor =
AnchorData{Url: anchor.Url, DataHash: hex.EncodeToString(anchor.DataHash[:])};
otherwise leave data.Anchor zero-valued (or set to nil/empty as per surrounding
logic) to match the nil-check pattern used for anchors elsewhere in this file.
🧹 Nitpick comments (1)
input/chainsync/chainsync.go (1)
601-620: LGTM! Governance event emission integrated correctly.The implementation properly checks for governance data and emits governance events alongside transaction events. The logic is consistent with the
handleRollForwardimplementation at lines 494-513.♻️ Optional: Consider extracting common governance event creation logic
The governance event creation logic is duplicated between
handleRollForward(lines 494-513) andhandleBlockFetchBlock(lines 601-620). If this pattern extends to more locations in the future, consider extracting it into a helper function:func (c *ChainSync) createGovernanceEventIfNeeded( block ledger.Block, transaction ledger.Transaction, index uint32, ) *event.Event { if !event.HasGovernanceData(transaction) { return nil } govEvt := event.New( "chainsync.governance", time.Now(), event.NewGovernanceContext( block, transaction, index, c.networkMagic, ), event.NewGovernanceEvent( block, transaction, c.includeCbor, ), ) return &govEvt }Then use it as:
if govEvt := c.createGovernanceEventIfNeeded(block, transaction, uint32(t)); govEvt != nil { tmpEvents = append(tmpEvents, *govEvt) }Note: This refactor is entirely optional and can be deferred.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
event/governance.goinput/chainsync/chainsync.go
🧰 Additional context used
🧬 Code graph analysis (1)
input/chainsync/chainsync.go (1)
event/governance.go (3)
HasGovernanceData(152-165)NewGovernanceContext(110-123)NewGovernanceEvent(126-149)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
input/chainsync/chainsync.go (1)
494-513: LGTM! Governance event emission integrated correctly.The implementation properly checks for governance data, creates both context and event with appropriate parameters, and appends to the event list. The bounds check on lines 474-476 justifies the safe conversion to
uint32.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="event/governance.go">
<violation number="1" location="event/governance.go:340">
P2: Default case returns `"ParameterChange"` instead of `"Unknown"`, which is inconsistent with `getVoterType` and `getVoteString` in this file. If `ParameterChangeGovAction` exists in the library, it should be handled explicitly. Using `"ParameterChange"` as default could silently misclassify unknown governance action types.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
event/governance.go
Outdated
| default: | ||
| return "ParameterChange" |
There was a problem hiding this comment.
P2: Default case returns "ParameterChange" instead of "Unknown", which is inconsistent with getVoterType and getVoteString in this file. If ParameterChangeGovAction exists in the library, it should be handled explicitly. Using "ParameterChange" as default could silently misclassify unknown governance action types.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At event/governance.go, line 340:
<comment>Default case returns `"ParameterChange"` instead of `"Unknown"`, which is inconsistent with `getVoterType` and `getVoteString` in this file. If `ParameterChangeGovAction` exists in the library, it should be handled explicitly. Using `"ParameterChange"` as default could silently misclassify unknown governance action types.</comment>
<file context>
@@ -0,0 +1,400 @@
+ return "NewConstitution"
+ case *lcommon.InfoGovAction:
+ return "Info"
+ default:
+ return "ParameterChange"
+ }
</file context>
| default: | |
| return "ParameterChange" | |
| case *lcommon.ParameterChangeGovAction: | |
| return "ParameterChange" | |
| default: | |
| return "Unknown" |
wolf31o2
force-pushed
the
feat/governance-event-type
branch
from
53b9b2c to
1956133
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @event/governance.go:
- Around line 238-323: The code uses two different anchor-presence checks
(c.Anchor != nil in extractGovernanceCertificates vs anchor.Url != "" in
extractProposalProcedures); standardize to the safer nil check: update
extractProposalProcedures to test anchor != nil before accessing its fields (and
create AnchorData only when non-nil), and ensure any other anchor.Url checks are
replaced with nil checks to avoid potential nil pointer dereferences;
references: function extractGovernanceCertificates, extractProposalProcedures,
and the Anchor/anchor or c.Anchor variables.
- Around line 327-344: getGovActionType currently falls back to returning
"ParameterChange" for any unknown lcommon.GovAction, which can mislabel new
action types; update getGovActionType to explicitly handle the ParameterChange
type (e.g., add a case for *lcommon.ParameterChangeGovAction if that type
exists) and change the default branch to return "Unknown" (and optionally log
the unexpected action type) so future/unknown actions are not misclassified.
- Around line 183-207: The cast to uint32 in extractProposalProcedures for the
Index field assumes proposal count fits in 32 bits and the nolint justification
is incorrect; update extractProposalProcedures to explicitly validate the number
of proposals before casting (e.g., check len(proposals) <= math.MaxUint32 and
handle the overflow case by returning an error, logging and truncating, or using
a larger index type), remove or replace the nolint comment with a clear comment
explaining the chosen behavior, and ensure any callers of
extractProposalProcedures handle the new overflow/validation behavior
accordingly.
🧹 Nitpick comments (1)
event/governance.go (1)
209-236: Consider pre-sizing the result slice if performance matters.The function uses
var result []VotingProcedureDatawhich starts with zero capacity and grows dynamically. If voting procedures are common, pre-calculating the size and usingmake()with an initial capacity could reduce allocations, though this would require iterating the map twice.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
event/governance.goinput/chainsync/chainsync.go
🚧 Files skipped from review as they are similar to previous changes (1)
- input/chainsync/chainsync.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
event/governance.go (5)
24-107: LGTM! Well-structured type definitions.The governance event types are well-organized with appropriate JSON tags. The separation of different certificate types into distinct data structures provides good clarity.
109-149: LGTM! Clean constructor implementations.The constructors follow a clear pattern and properly initialize the governance event structures.
151-181: LGTM! Comprehensive governance data detection.The detection logic properly checks all governance data sources (voting procedures, proposal procedures, and governance certificates).
346-371: LGTM! Good use of "Unknown" defaults.The helper functions properly map enumeration values to strings with sensible defaults.
373-401: The code correctly implements DRep ID encoding. Lines 389 and 393 uselcommon.NewBlake2b224(drep.Credential).Bech32()becausedrep.Credentialis raw bytes (28-byte Blake2b-224 hash), whereas DRep certificates use.Hash().Bech32()because they receivelcommon.Credentialobjects. Both patterns are correct for their respective input types. The "drep" and "drep_script" prefixes match Cardano's Bech32 encoding standards for key-hash and script-hash DRep credentials (per CIP-0129/0105). No changes needed.
| func extractGovernanceCertificates(tx ledger.Transaction) ( | ||
| drep []DRepCertificateData, | ||
| voteDel []VoteDelegationCertificateData, | ||
| committee []CommitteeCertificateData, | ||
| ) { | ||
| for _, cert := range tx.Certificates() { | ||
| switch c := cert.(type) { | ||
| // DRep certificates | ||
| case *lcommon.RegistrationDrepCertificate: | ||
| data := DRepCertificateData{ | ||
| CertificateType: "Registration", | ||
| DRepHash: hex.EncodeToString(c.DrepCredential.Hash().Bytes()), | ||
| DRepId: c.DrepCredential.Hash().Bech32("drep"), | ||
| Deposit: c.Amount, | ||
| } | ||
| if c.Anchor != nil { | ||
| data.Anchor = AnchorData{ | ||
| Url: c.Anchor.Url, | ||
| DataHash: hex.EncodeToString(c.Anchor.DataHash[:]), | ||
| } | ||
| } | ||
| drep = append(drep, data) | ||
|
|
||
| case *lcommon.DeregistrationDrepCertificate: | ||
| drep = append(drep, DRepCertificateData{ | ||
| CertificateType: "Deregistration", | ||
| DRepHash: hex.EncodeToString(c.DrepCredential.Hash().Bytes()), | ||
| DRepId: c.DrepCredential.Hash().Bech32("drep"), | ||
| Deposit: c.Amount, | ||
| }) | ||
|
|
||
| case *lcommon.UpdateDrepCertificate: | ||
| data := DRepCertificateData{ | ||
| CertificateType: "Update", | ||
| DRepHash: hex.EncodeToString(c.DrepCredential.Hash().Bytes()), | ||
| DRepId: c.DrepCredential.Hash().Bech32("drep"), | ||
| } | ||
| if c.Anchor != nil { | ||
| data.Anchor = AnchorData{ | ||
| Url: c.Anchor.Url, | ||
| DataHash: hex.EncodeToString(c.Anchor.DataHash[:]), | ||
| } | ||
| } | ||
| drep = append(drep, data) | ||
|
|
||
| // Vote delegation certificates | ||
| case *lcommon.VoteDelegationCertificate: | ||
| voteDel = append(voteDel, extractVoteDelegation("VoteDelegation", c.StakeCredential, c.Drep, 0)) | ||
|
|
||
| case *lcommon.StakeVoteDelegationCertificate: | ||
| data := extractVoteDelegation("StakeVoteDelegation", c.StakeCredential, c.Drep, 0) | ||
| data.PoolKeyHash = hex.EncodeToString(c.PoolKeyHash.Bytes()) | ||
| voteDel = append(voteDel, data) | ||
|
|
||
| case *lcommon.VoteRegistrationDelegationCertificate: | ||
| voteDel = append(voteDel, extractVoteDelegation("VoteRegistrationDelegation", c.StakeCredential, c.Drep, c.Amount)) | ||
|
|
||
| case *lcommon.StakeVoteRegistrationDelegationCertificate: | ||
| data := extractVoteDelegation("StakeVoteRegistrationDelegation", c.StakeCredential, c.Drep, c.Amount) | ||
| data.PoolKeyHash = hex.EncodeToString(c.PoolKeyHash.Bytes()) | ||
| voteDel = append(voteDel, data) | ||
|
|
||
| // Constitutional Committee certificates | ||
| case *lcommon.AuthCommitteeHotCertificate: | ||
| committee = append(committee, CommitteeCertificateData{ | ||
| CertificateType: "AuthHot", | ||
| ColdCredential: hex.EncodeToString(c.ColdCredential.Hash().Bytes()), | ||
| HotCredential: hex.EncodeToString(c.HotCredential.Hash().Bytes()), | ||
| }) | ||
|
|
||
| case *lcommon.ResignCommitteeColdCertificate: | ||
| data := CommitteeCertificateData{ | ||
| CertificateType: "ResignCold", | ||
| ColdCredential: hex.EncodeToString(c.ColdCredential.Hash().Bytes()), | ||
| } | ||
| if c.Anchor != nil { | ||
| data.Anchor = AnchorData{ | ||
| Url: c.Anchor.Url, | ||
| DataHash: hex.EncodeToString(c.Anchor.DataHash[:]), | ||
| } | ||
| } | ||
| committee = append(committee, data) | ||
| } | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Note: Inconsistent anchor checking pattern.
Lines 253, 275, and 313 check c.Anchor != nil, while line 198 in extractProposalProcedures checks anchor.Url != "". Both approaches work, but consider standardizing on one pattern for consistency. Checking != nil is generally safer as it prevents nil pointer dereferences.
Otherwise, the comprehensive certificate extraction logic correctly handles all governance certificate types.
🤖 Prompt for AI Agents
In @event/governance.go around lines 238 - 323, The code uses two different
anchor-presence checks (c.Anchor != nil in extractGovernanceCertificates vs
anchor.Url != "" in extractProposalProcedures); standardize to the safer nil
check: update extractProposalProcedures to test anchor != nil before accessing
its fields (and create AnchorData only when non-nil), and ensure any other
anchor.Url checks are replaced with nil checks to avoid potential nil pointer
dereferences; references: function extractGovernanceCertificates,
extractProposalProcedures, and the Anchor/anchor or c.Anchor variables.
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
feat/governance-event-type
branch
from
1956133 to
41f03a3
Compare
|
@cubic-dev-ai review please |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
2 participants
Closes #487
Summary by cubic
Add a governance event type and emit it from ChainSync when a transaction contains governance data. This lets consumers index proposals, votes, DRep changes, delegations, and committee actions, with optional tx CBOR.
Written for commit 41f03a3. Summary will update on new commits.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.