Skip to content

Conversation

@jkawan
Copy link
Contributor

@jkawan jkawan commented Oct 26, 2025

  1. Implemented ProposedProtocolParamsUpdatesResult type from any to [map[]]

Command Prompted:
root@5d4baeec65b1:/code# go run ./cmd/gouroboros/ -network devnet -network-magic 42 -socket /ipc/node.socket query proposed-protocol-params-updates
Output:
proposed-protocol-params-updates: [map[]]

Closes #861

Summary by CodeRabbit

  • New Features
    • Added support for querying proposed protocol parameter updates, enabling users to retrieve information about upcoming protocol changes before they are activated on the network.

Signed-off-by: Jenita <jkawan@blinklabs.io>
@jkawan jkawan requested a review from a team as a code owner October 26, 2025 20:05
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 26, 2025

📝 Walkthrough

Walkthrough

Implemented ProposedProtocolParamsUpdatesResult feature by adding a new query command to retrieve proposed protocol parameter updates, defining a concrete return type as a map keyed by genesis hash, and introducing a GenesisHash type alias for improved type safety.

Changes

Cohort / File(s) Summary
Query Command Addition
cmd/gouroboros/query.go
Added "proposed-protocol-params-updates" query case that invokes GetProposedProtocolParamsUpdates, handles errors, and outputs results
Protocol Query Type Definition
protocol/localstatequery/queries.go
Updated ProposedProtocolParamsUpdatesResult signature from any to map[common.GenesisHash]common.ProtocolParameterUpdate and added import for common types
Common Type Alias
ledger/common/common.go
Added GenesisHash type alias representing Blake2b-224 hash for genesis keys

Sequence Diagram

sequenceDiagram
    participant User
    participant QueryCmd as Query Command
    participant LSQ as LocalStateQuery Client
    participant ResultMap as ProposedProtocolParamsUpdatesResult

    User->>QueryCmd: Execute "proposed-protocol-params-updates"
    QueryCmd->>LSQ: Call GetProposedProtocolParamsUpdates()
    alt Success
        LSQ-->>ResultMap: map[GenesisHash]ProtocolParameterUpdate
        ResultMap-->>QueryCmd: Structured results
        QueryCmd->>User: Print updates
    else Error
        LSQ-->>QueryCmd: Error
        QueryCmd->>User: Print error & exit
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Straightforward query command addition following existing patterns
  • Type signature change from any to concrete type requires verifying the map structure is correct and matches upstream dependencies
  • Type alias is trivial and self-contained

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues Check ✅ Passed The pull request successfully implements the ProposedProtocolParamsUpdatesResult type as required by issue #861. The changes update the type signature from any to map[common.GenesisHash]common.ProtocolParameterUpdate, add the necessary GenesisHash type alias in the common package, import the required dependencies, and implement the query case "proposed-protocol-params-updates" to support the functionality. All coding-related objectives from the linked issue appear to be met by these changes.
Out of Scope Changes Check ✅ Passed All changes in the pull request are directly scoped to implementing the ProposedProtocolParamsUpdatesResult type and its supporting functionality. The GenesisHash type alias, the updated type signature, the new import, and the query case implementation are all necessary components for fulfilling the stated objective. No unrelated or extraneous changes were introduced that deviate from the requirements specified in issue #861.
Title Check ✅ Passed The title "fix(protocol): Implemented ProposedProtocolParamsUpdatesResult type" directly references the main change in the pull request, which is updating the ProposedProtocolParamsUpdatesResult type from a vague any type to a properly typed map[common.GenesisHash]common.ProtocolParameterUpdate. The title is clear, specific, and uses appropriate semantic versioning prefix ("fix") to indicate the nature of the change. The phrasing is concise and allows developers scanning the history to understand that the PR improves type safety for protocol parameter update results.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat-proposed-protocl-params

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
cmd/gouroboros/query.go (1)

308-314: Consider enhanced output formatting for better readability.

The implementation follows the established pattern and is functionally correct. To improve the user experience, consider using JSON formatting for the map output:

 case "proposed-protocol-params-updates":
 	proposedUpdates, err := o.LocalStateQuery().Client.GetProposedProtocolParamsUpdates()
 	if err != nil {
 		fmt.Printf("ERROR: failure querying proposed protocol params updates: %s\n", err)
 		os.Exit(1)
 	}
-	fmt.Printf("proposed-protocol-params-updates: %v\n", *proposedUpdates)
+	updatesJson, err := json.MarshalIndent(*proposedUpdates, "", "  ")
+	if err != nil {
+		fmt.Printf("ERROR: failed to marshal updates: %s\n", err)
+		os.Exit(1)
+	}
+	fmt.Printf("proposed-protocol-params-updates: %s\n", updatesJson)

This would provide more readable output for complex map structures, especially when multiple pools have proposed updates.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between becb83c and 5b0f5aa.

📒 Files selected for processing (2)
  • cmd/gouroboros/query.go (1 hunks)
  • protocol/localstatequery/queries.go (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
protocol/localstatequery/queries.go (1)
ledger/common/pparams.go (1)
  • ProtocolParameterUpdate (24-27)
cmd/gouroboros/query.go (2)
protocol/localstatequery/localstatequery.go (1)
  • LocalStateQuery (116-119)
protocol/localstatequery/client.go (1)
  • Client (30-43)

Signed-off-by: Jenita <jkawan@blinklabs.io>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
ledger/common/common.go (1)

449-463: Consider adding MarshalJSON method for JSON serialization consistency.

The GenesisKeyHash type follows the basic pattern with constructor, String(), and Bytes() methods. However, comparing to the similar Blake2b224 type (also 28 bytes, lines 74-96), GenesisKeyHash is missing a MarshalJSON method. Given that this type appears to be used in protocol parameter updates that are likely serialized to JSON for CLI output, adding MarshalJSON would ensure consistent behavior when the type is used as a map key or value in JSON contexts.

Consider adding the following method for JSON serialization consistency:

 func (g GenesisKeyHash) Bytes() []byte {
 	return g[:]
 }
+
+func (g GenesisKeyHash) MarshalJSON() ([]byte, error) {
+	return json.Marshal(g.String())
+}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5b0f5aa and 3a89f93.

📒 Files selected for processing (3)
  • ledger/common/common.go (1 hunks)
  • ledger/compat.go (1 hunks)
  • protocol/localstatequery/queries.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • protocol/localstatequery/queries.go
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/common/common.go (1)
ledger/compat.go (2)
  • NewGenesisKeyHash (85-87)
  • GenesisKeyHash (78-78)
ledger/compat.go (2)
ledger/common/certs.go (1)
  • PoolRelay (356-362)
ledger/common/common.go (3)
  • PoolId (415-415)
  • GenesisKeyHash (449-449)
  • NewGenesisKeyHash (451-455)
⏰ 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 (1)
ledger/compat.go (1)

76-87: LGTM! Compatibility layer correctly exposes GenesisKeyHash.

The type alias and constructor function properly expose common.GenesisKeyHash through the compatibility layer, following the established pattern used for other types like PoolId. The placement in the "Pools" section alongside PoolId is logical given their similar nature (both are 28-byte identifiers).

Signed-off-by: Jenita <jkawan@blinklabs.io>
Signed-off-by: Jenita <jkawan@blinklabs.io>
@wolf31o2 wolf31o2 dismissed agaffney’s stale review October 31, 2025 17:55

Requested changes have been made

@wolf31o2 wolf31o2 changed the title Implemented ProposedProtocolParamsUpdatesResult type fix(protocol): Implemented ProposedProtocolParamsUpdatesResult type Oct 31, 2025
@wolf31o2 wolf31o2 merged commit 51035ad into main Oct 31, 2025
10 checks passed
@wolf31o2 wolf31o2 deleted the feat-proposed-protocl-params branch October 31, 2025 17:56
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.

Implement ProposedProtocolParamsUpdatesResult

4 participants