Skip to content
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

chore(server/v2/cometbft): simplify getProtoRegistry (backport #22248) #22260

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Oct 14, 2024

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 change

  • targeted 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.md

  • updated 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...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Enhanced encoding and decoding capabilities for Quad instances, including support for non-terminal keys.
    • Improved schema representation for Quads.
  • Bug Fixes

    • Updated error handling in the indexer for better error message formatting.
    • Added resource management for closing response bodies in the chain registry.
  • Refactor

    • Simplified initialization of the getProtoRegistry field in the Consensus struct.
    • Removed unnecessary JSON sorting function from tests.

This is an automatic backport of pull request #22248 done by [Mergify](https://mergify.com).

(cherry picked from commit 4274dcf)

# Conflicts:
#	collections/quad.go
#	tools/hubl/internal/registry.go
@mergify mergify bot requested a review from a team as a code owner October 14, 2024 21:30
@mergify mergify bot added the conflicts label Oct 14, 2024
Copy link
Contributor Author

mergify bot commented Oct 14, 2024

Cherry-pick of 4274dcf has failed:

On branch mergify/bp/release/v0.52.x/pr-22248
Your branch is up to date with 'origin/release/v0.52.x'.

You are currently cherry-picking commit 4274dcf44.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   server/v2/cometbft/abci.go

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	deleted by us:   collections/quad.go
	deleted by us:   tools/hubl/internal/registry.go

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

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

Only the CometBFT changes should be backported

Comment on lines 272 to 279
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)
Copy link

Choose a reason for hiding this comment

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

Seeing the SizeNonTerminal later in the code I guess this was copy pasted but should be using size?

Suggested change
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)

Comment on lines 247 to 257
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:])
Copy link

Choose a reason for hiding this comment

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

Similar as previous comment. Also k4 seems to be using the good function

Suggested change
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:])

Comment on lines 27 to 30
if t.k1 != nil {
return *t.k1
}
return x
Copy link

Choose a reason for hiding this comment

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

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)


// 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)
Copy link

Choose a reason for hiding this comment

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

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).

}

func GetChainRegistryEntry(chain string) (*ChainRegistryEntry, error) {
res, err := http.Get(fmt.Sprintf("https://raw.githubusercontent.com/cosmos/chain-registry/master/%v/chain.json", chain))
Copy link

Choose a reason for hiding this comment

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

nit

Suggested change
res, err := http.Get(fmt.Sprintf("https://raw.githubusercontent.com/cosmos/chain-registry/master/%v/chain.json", chain))
res, err := http.Get(fmt.Sprintf("https://raw.githubusercontent.com/cosmos/chain-registry/master/%s/chain.json", chain))

return nil, err
}

if err = res.Body.Close(); err != nil {
Copy link

Choose a reason for hiding this comment

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

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.

}

data := &ChainRegistryEntry{}
if err = json.Unmarshal(bz, data); err != nil {
Copy link

Choose a reason for hiding this comment

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

what about using straight json.NewDeocder(res).Decode(data)

}

// clean-up the URL
cleanEntries := make([]*APIEntry, 0)
Copy link

Choose a reason for hiding this comment

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

nit: capacity could already be allocated

Suggested change
cleanEntries := make([]*APIEntry, 0)
cleanEntries := make([]*APIEntry, 0, len(data.APIs.GRPC)

Comment on lines 55 to 62
if !strings.Contains(data.APIs.GRPC[i].Address, ":") {
continue
}

cleanEntries = append(cleanEntries, data.APIs.GRPC[i])
}

data.APIs.GRPC = cleanEntries
Copy link

Choose a reason for hiding this comment

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

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:)...)
}

Comment on lines 47 to 58
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
}

Copy link

Choose a reason for hiding this comment

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

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 julienrbrt changed the title refactor: add close body after use and fix lint (backport #22248) chore(server/v2/cometbft): simplify getProtoRegistry (backport #22248) Oct 15, 2024
@julienrbrt julienrbrt merged commit 5f534c9 into release/v0.52.x Oct 15, 2024
19 of 68 checks passed
@julienrbrt julienrbrt deleted the mergify/bp/release/v0.52.x/pr-22248 branch October 15, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:server/v2 cometbft C:server/v2 Issues related to server/v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants