chore(server/v2/cometbft): simplify getProtoRegistry (backport #22248) #22260
Conversation
![mergify[bot]](https://github.com/avatars/in/10562?s=60&v=4){kind=small}
(cherry picked from commit 4274dcf) # Conflicts: # collections/quad.go # tools/hubl/internal/registry.go
|
Cherry-pick of 4274dcf has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
github-actions
bot
added
C:collections
C:Hubl
collections/quad.go
Outdated
| if key.k1 != nil { | ||
| size += t.keyCodec1.SizeNonTerminal(*key.k1) | ||
| } | ||
| if key.k2 != nil { | ||
| size += t.keyCodec2.SizeNonTerminal(*key.k2) | ||
| } | ||
| if key.k3 != nil { | ||
| size += t.keyCodec3.SizeNonTerminal(*key.k3) |
There was a problem hiding this comment.
Seeing the SizeNonTerminal later in the code I guess this was copy pasted but should be using size?
| if key.k1 != nil { | |
| size += t.keyCodec1.SizeNonTerminal(*key.k1) | |
| } | |
| if key.k2 != nil { | |
| size += t.keyCodec2.SizeNonTerminal(*key.k2) | |
| } | |
| if key.k3 != nil { | |
| size += t.keyCodec3.SizeNonTerminal(*key.k3) | |
| if key.k1 != nil { | |
| size += t.keyCodec1.Size(*key.k1) | |
| } | |
| if key.k2 != nil { | |
| size += t.keyCodec2.Size(*key.k2) | |
| } | |
| if key.k3 != nil { | |
| size += t.keyCodec3.Size(*key.k3) |
collections/quad.go
Outdated
| read, key1, err := t.keyCodec1.DecodeNonTerminal(buffer) | ||
| if err != nil { | ||
| return 0, Quad[K1, K2, K3, K4]{}, err | ||
| } | ||
| readTotal += read | ||
| read, key2, err := t.keyCodec2.DecodeNonTerminal(buffer[readTotal:]) | ||
| if err != nil { | ||
| return 0, Quad[K1, K2, K3, K4]{}, err | ||
| } | ||
| readTotal += read | ||
| read, key3, err := t.keyCodec3.DecodeNonTerminal(buffer[readTotal:]) |
There was a problem hiding this comment.
Similar as previous comment. Also k4 seems to be using the good function
| read, key1, err := t.keyCodec1.DecodeNonTerminal(buffer) | |
| if err != nil { | |
| return 0, Quad[K1, K2, K3, K4]{}, err | |
| } | |
| readTotal += read | |
| read, key2, err := t.keyCodec2.DecodeNonTerminal(buffer[readTotal:]) | |
| if err != nil { | |
| return 0, Quad[K1, K2, K3, K4]{}, err | |
| } | |
| readTotal += read | |
| read, key3, err := t.keyCodec3.DecodeNonTerminal(buffer[readTotal:]) | |
| read, key1, err := t.keyCodec1.Decodel(buffer) | |
| if err != nil { | |
| return 0, Quad[K1, K2, K3, K4]{}, err | |
| } | |
| readTotal += read | |
| read, key2, err := t.keyCodec2.Decode(buffer[readTotal:]) | |
| if err != nil { | |
| return 0, Quad[K1, K2, K3, K4]{}, err | |
| } | |
| readTotal += read | |
| read, key3, err := t.keyCodec3.Decode(buffer[readTotal:]) |
collections/quad.go
Outdated
| if t.k1 != nil { | ||
| return *t.k1 | ||
| } | ||
| return x |
There was a problem hiding this comment.
nit: I would recommend using some utility function like
func pointerToValue[T any](p *T) (v T) {
if p != nil {
return *p
}
return v
}Or use an already library having such functions (there are some, imo, dangerous dereferencing later in the code that could benefit from it too)
collections/quad.go
Outdated
|
|
||
| // EncodeJSON encodes Quads to json | ||
| func (t quadKeyCodec[K1, K2, K3, K4]) EncodeJSON(value Quad[K1, K2, K3, K4]) ([]byte, error) { | ||
| json1, err := t.keyCodec1.EncodeJSON(*value.k1) |
There was a problem hiding this comment.
value.kn might be nil, I would recommend to either check if before or to use a safe function to get the value out of the pointer (see previous comment).
tools/hubl/internal/registry.go
Outdated
| } | ||
|
|
||
| func GetChainRegistryEntry(chain string) (*ChainRegistryEntry, error) { | ||
| res, err := http.Get(fmt.Sprintf("https://github.com/raw/cosmos/chain-registry/master/%v/chain.json", chain)) |
There was a problem hiding this comment.
nit
| res, err := http.Get(fmt.Sprintf("https://github.com/raw/cosmos/chain-registry/master/%v/chain.json", chain)) | |
| res, err := http.Get(fmt.Sprintf("https://github.com/raw/cosmos/chain-registry/master/%s/chain.json", chain)) |
tools/hubl/internal/registry.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| if err = res.Body.Close(); err != nil { |
There was a problem hiding this comment.
If the io.ReadAll operation panics or returns and error the body won't be closed so, Imo, this should be put in a defer statement right after the err check L26.
tools/hubl/internal/registry.go
Outdated
| } | ||
|
|
||
| data := &ChainRegistryEntry{} | ||
| if err = json.Unmarshal(bz, data); err != nil { |
There was a problem hiding this comment.
what about using straight json.NewDeocder(res).Decode(data)
tools/hubl/internal/registry.go
Outdated
| } | ||
|
|
||
| // clean-up the URL | ||
| cleanEntries := make([]*APIEntry, 0) |
There was a problem hiding this comment.
nit: capacity could already be allocated
| cleanEntries := make([]*APIEntry, 0) | |
| cleanEntries := make([]*APIEntry, 0, len(data.APIs.GRPC) |
tools/hubl/internal/registry.go
Outdated
| if !strings.Contains(data.APIs.GRPC[i].Address, ":") { | ||
| continue | ||
| } | ||
|
|
||
| cleanEntries = append(cleanEntries, data.APIs.GRPC[i]) | ||
| } | ||
|
|
||
| data.APIs.GRPC = cleanEntries |
There was a problem hiding this comment.
in the end this piece will just remove the "bad" entries from the data.APIs.GRPC slice. There is maybe no need for the cleanEntries at all but rather something like
if !strings.Contains(data.APIs.GRPC[i].Address, ":") {
data.APIs.GRPC = append(data.APIs.GRPC[:i], data.APIs.GRPC[s+1:)...)
}
tools/hubl/internal/registry.go
Outdated
| if idx := strings.Index(apiEntry.Address, "://"); idx != -1 { | ||
| data.APIs.GRPC[i].Address = apiEntry.Address[idx+3:] | ||
| } | ||
|
|
||
| // remove trailing slashes | ||
| data.APIs.GRPC[i].Address = strings.TrimSuffix(data.APIs.GRPC[i].Address, "/") | ||
|
|
||
| // remove addresses without a port | ||
| if !strings.Contains(data.APIs.GRPC[i].Address, ":") { | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
I believe using the net/url would be safer here. Also please add unit tests.
I think that a string such as ://://somerubbish:abcd//123// will be successfully stored as ://somerubbish:abcd//123. With the url package it would fail. In the end I think you would probably only want to keep the host from the address (playground: https://go.dev/play/p/X74iDrAnTTp) + perhaps path+rawQuery+fragment depending on the intent
julienrbrt
changed the title
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
included the correct type prefix in the PR title, you can find examples of the prefixes below:
confirmed
!in the type prefix if API or client breaking changetargeted the correct branch (see PR Targeting)
provided a link to the relevant issue or specification
reviewed "Files changed" and left comments if necessary
included the necessary unit and integration tests
added a changelog entry to
CHANGELOG.mdupdated the relevant documentation or specification, including comments for documenting Go code
confirmed all CI checks have passed
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
getProtoRegistryfield in the Consensus struct.This is an automatic backport of pull request #22248 done by [Mergify](https://mergify.com).