-
Notifications
You must be signed in to change notification settings - Fork 21
fix(protocol): Implemented ProposedProtocolParamsUpdatesResult type #1214
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
Conversation
Signed-off-by: Jenita <jkawan@blinklabs.io>
📝 WalkthroughWalkthroughImplemented 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
Sequence DiagramsequenceDiagram
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
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
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.
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
📒 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>
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ledger/common/common.go (1)
449-463: Consider adding MarshalJSON method for JSON serialization consistency.The
GenesisKeyHashtype follows the basic pattern with constructor,String(), andBytes()methods. However, comparing to the similarBlake2b224type (also 28 bytes, lines 74-96),GenesisKeyHashis missing aMarshalJSONmethod. Given that this type appears to be used in protocol parameter updates that are likely serialized to JSON for CLI output, addingMarshalJSONwould 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
📒 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.GenesisKeyHashthrough the compatibility layer, following the established pattern used for other types likePoolId. The placement in the "Pools" section alongsidePoolIdis logical given their similar nature (both are 28-byte identifiers).
Signed-off-by: Jenita <jkawan@blinklabs.io>
Signed-off-by: Jenita <jkawan@blinklabs.io>
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