Flatten sei-tendermint go mod into sei-chain#2817
Conversation
Flatten the sei-tendermint go module into sei-chain tin reduce dependency management complexity and error-prone multi layer replace directives. Part of PLT-6
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2817 +/- ##
===========================================
- Coverage 56.77% 48.13% -8.65%
===========================================
Files 2070 666 -1404
Lines 168274 50121 -118153
===========================================
- Hits 95542 24126 -71416
+ Misses 64247 23885 -40362
+ Partials 8485 2110 -6375
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
* errcheck: 50 * goconst: 1 * goimports: 3 * gosec: 50 * ineffassign: 2 * misspell: 2 * prealloc: 17 * staticcheck: 45 * unconvert: 1
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2817 +/- ##
==========================================
+ Coverage 52.22% 57.11% +4.88%
==========================================
Files 2027 2088 +61
Lines 160424 171077 +10653
==========================================
+ Hits 83778 97705 +13927
+ Misses 68732 64693 -4039
- Partials 7914 8679 +765
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| dumpDebugData(ctx, logger, rpc, dumpArgs) | ||
| } | ||
| ticker := time.NewTicker(time.Duration(frequency) * time.Second) //nolint: gosec | ||
| defer ticker.Stop() |
There was a problem hiding this comment.
nit: not needed since 1.23
There was a problem hiding this comment.
Although it is not strictly needed after 1.23, there are a few reasons to still call stop: communication of intent, defensive programming in case of refactoring that may move the ticker and so on.
But the main functional reason to call it is that it would stop the ticker immediately, instead of leaving it to GC. It makes up a more predictable code behaviour.
| @@ -153,19 +153,19 @@ func ProofFromProto(pb *tmcrypto.Proof) (*Proof, error) { | |||
| // Recursive impl. | |||
| func computeHashFromAunts(index, total int64, leafHash []byte, innerHashes [][]byte) ([]byte, error) { | |||
| if index >= total || index < 0 || total <= 0 { | |||
| return nil, fmt.Errorf("Calling computeHashFromAunts() with invalid index (%d) and total (%d)", index, total) | |||
| return nil, fmt.Errorf("calling computeHashFromAunts() with invalid index (%d) and total (%d)", index, total) | |||
There was a problem hiding this comment.
fyi, the lint which checks that the first letter of the error is lowercase is imprecise and super annoying - sometimes the first letter simply has to be uppercase (like when the first word is a name of a public type, or sth). I'd recommend to disable it.
There was a problem hiding this comment.
Agreed that ST1005 can be annoying, but I'd push back on disabling it entirely; the consistency alone is worth it. The general practice is to instead capitalise the first letter in logs, because they typically are a list of sentences. See:
The "(unless beginning with proper nouns or acronyms)" is loose. As a reference, I very rarely come across Go SDK implementation that falls in that exceptional category.
In this specific case, the error should indeed start with lower case.
There was a problem hiding this comment.
I disagree that it is worth it, but I can work with that - in worst case I can prefix every message with "q".
There was a problem hiding this comment.
Glad to hear that. Also Go community is pretty active, maybe you could submit a proposal and hear what the community has to say on this.
| @@ -336,7 +336,7 @@ func newValidBlockMessageFromProto(pb *tmcons.NewValidBlock) (*NewValidBlockMess | |||
| func proposalMessageFromProto(pb *tmcons.Proposal) (*ProposalMessage, error) { | |||
| proposal, err := types.ProposalFromProto(&pb.Proposal) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("Proposal: %w", err) | |||
There was a problem hiding this comment.
like here: the field is "Proposal", not "proposal". Lowercasing is not helpful.
There was a problem hiding this comment.
Sure, we can keep it uppercase and add a lint ignore. But looking at the repo, the error strings are already inconsistent. Rather than adding lint ignores case by case, I'd suggest adopting a consistent "verb noun/Noun: %w" pattern; example
fmt.Errorf("converting Proposal: %w", err)This reads well when errors chain and eventually logged; e.g. :
Failed to process peer message: converting Proposal: invalid signature length
vs.
Failed to process peer message: Proposal: invalid signature length
...and naturally avoids the capitalization lint issue entirely. It also matches the Go stdlib pattern (e.g. "tls: failed to find certificate...") where error strings compose mid-sentence as described in the Go Code Review Comments.
This repo doesn't have a logging convention yet. Adopting "verb noun/Noun/WhatEver: %w" now means fewer lint ignores going forward and clearer log lines for debugging.
There was a problem hiding this comment.
as far as I'm concerned verbs are almost always meaningless - all we care in practice is a stack trace, and mangling it into a human readable message is just another layer of obfuscation that we have to get through to actually locate where the error comes from.
There was a problem hiding this comment.
as far as I'm concerned verbs are almost always meaningless -
A tad over generalised 😅
actually locate where the error comes from.
Yep; logging is a mess all over the repo and this is a great test to make sure logging is useful.
| @@ -394,7 +395,7 @@ func (ps *PeerState) ApplyNewRoundStepMessage(msg *NewRoundStepMessage) { | |||
| defer ps.mtx.Unlock() | |||
|
|
|||
| // ignore duplicates or decreases | |||
| if msg.HRS.Cmp(ps.PRS.HRS) <= 0 { | |||
| if msg.Cmp(ps.PRS.HRS) <= 0 { | |||
There was a problem hiding this comment.
this lint does something weird - we compare HRS embedded fields here. Hiding that fact harms readability.
There was a problem hiding this comment.
This is from QF1008 staticcheck linter. Its rationale is rooted in Effective Go, where the entire point of embedding is promotion; to avoid bookkeeping and decouple from internals.
I'm against always using the explicit form, because that defeats the purpose of the language feature. In this case, HRS is a core type with no clashing receiver functions on both embedder and embedded types in this repo.
If HRS is more than an implementation detail, then my recommendation is to not embed it; use a named field instead. That forces callers to always spell out "I want to compare X and Y by their progress in terms of height, round, step". What harms readability most is allowing two ways of doing the same thing when the intent is to always use one. Keeping QF1008 enabled helps enforce that consistency.
Happy to revert this change in any case.
There was a problem hiding this comment.
here embedding is used to group a bunch of fields, especially so that you can grab them and compare them against a group of fields of another struct. The point is that this call is asymmetric now - we promote the receiver but not the argument. And it will stop working if we define another Cmp on the msg itself.
There was a problem hiding this comment.
And it will stop working if we define another Cmp on the msg itself.
Correct. emphasis on if.
as I pointed out, defining HRS as a named struct field is the concrete way to make sure that can't happen in that the compiler will error out. Having it embedded, and worrying about overrides seems like a racy way to assert intent. I would encourage thinking deeper about the intent of embedding.
pompon0
left a comment
pompon0
left a comment
There was a problem hiding this comment.
LGTM for code in sei-tendermint dir.
# Conflicts: # go.mod # go.sum # go.work.sum # sei-cosmos/go.mod # sei-cosmos/go.sum
|
Landing this to avid further merge conflicts with |
4 participants
Flatten the sei-tendermint go module into sei-chain tin reduce dependency management complexity and error-prone multi layer replace directives.
Part of PLT-6