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

enable custom go linter in CI #25

Merged
merged 3 commits into from
Aug 12, 2024
Merged

Conversation

hopeyen
Copy link

@hopeyen hopeyen commented Aug 9, 2024

(disclaimer: I'm not usually this talkative with PR comments, being unusual because I'm new and trying to become familiar with things)

This PR contains minor refactoring to make the linter happy

For the custom go run ./linters ./..., the only complain was

 ✗ go run ./linters ./...
.../nitro/eigenda/eigenda.go:36:2: field eigenda.EigenDAConfig.Enable not used
.../nitro/eigenda/eigenda.go:37:2: field eigenda.EigenDAConfig.Rpc not used
exit status 3

As a new gopher, I've tried using unused: ignore-field in .golang.ci.yml, adding used:"true" as part of field definitions, //nolint:unused to disable check for the config struct, modifying the structinit.go analyzer, but nothing worked until I made a dumb initializer for the struct 🤦 If I'm missing anything obvious, please let me know and I will make smarter changes.
So then, reviewing that EigenDAConfig is only used for initializing EigenDAClient, I think it makes sense to have arbnode pass in the config struct instead of only the RPC field. If we want more granular control of client initailization in the future, this change is probably in the right direction. Also included an additional/duplicate check for Enable to make linter happy; I think we can delete this check in arbnode, but I'm nervous about safety in golang

golangci-lint

a couple of failures there, change explanations:

all files got go fmted.
go sec

arbnode/dataposter/data_poster.go:223:23: G402: TLS InsecureSkipVerify may be true. (gosec)
-> #nosec moved to group config (https://github.com/securego/gosec/issues/278#issuecomment-461251791)

^ this is okay when updated local to 1.59.1

go critic

eigenda/decoding.go:119:17: appendAssign: append result not assigned to the same slice (gocritic)
        encodedData := append(codecBlobHeader, rawDataPadded...)
                       ^
->  keep using append by assign it back to the original slice
eigenda/proxy.go:18:28: captLocal: `RPCUrl' should not be capitalized (gocritic)
func NewEigenDAProxyClient(RPCUrl string) *EigenDAProxyClient {
                           ^
-> replace `RPCUrl` with `rpcUrl`
arbnode/sequencer_inbox.go:178:23: unslice: could simplify calldata[:] to calldata (gocritic)
                data = append(data, calldata[:]...)
                                    ^
-> simplified

errorlint

eigenda/proxy.go:143:66: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
                return nil, fmt.Errorf("failed to construct http request: %e", err)
                                                            ^
-> %e to %w                                             

govet

eigenda/types.go:257:30: nilness: tautological condition: non-nil != nil (govet)
        if &e.BlobVerificationProof != nil {
                                    ^
eigenda/types.go:259:45: nilness: tautological condition: non-nil != nil (govet)
                if &e.BlobVerificationProof.BatchMetadata != nil {
                                                          ^
-> always tautological since taking the address of a struct field will always return a non-nil pointer even if the field itself is a nil pointer or has a zero value.
-> added IsEmpty helpers for structs and use that for the checks

🤞 let's see if CI will pass

@hopeyen hopeyen added the good first issue Good for newcomers label Aug 9, 2024
@hopeyen hopeyen self-assigned this Aug 9, 2024
@hopeyen hopeyen changed the base branch from develop to eigenda-v3.0.3 August 9, 2024 14:11
@hopeyen hopeyen force-pushed the hope/custom-go-lint branch from 2fe3a82 to 2d67a67 Compare August 9, 2024 15:52
@hopeyen
Copy link
Author

hopeyen commented Aug 9, 2024

Screenshot 2024-08-09 at 12 27 06 PM
all lint tests passed, only thing failing is docker build (local rpc not working)

resolves #7

@hopeyen hopeyen requested a review from epociask August 9, 2024 17:27
Copy link
Collaborator

@epociask epociask left a comment

Choose a reason for hiding this comment

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

Just one comment - otherwise LGTM

Comment on lines 47 to 49
if !config.Enable {
panic("EigenDA is not enabled")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why panic versus bubbling error up -- also given that there's a precursor check performed within the caller for the Enable flag we can probably get rid of this

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, updated to bubbling error up.

This check exists purely because of the linter's complain that Enable hasn't been used even with the caller's check, more detailed reasoning is in the PR comments, happy to learn about alternatives 🤔

Comment on lines +400 to +422
func (b BlobVerificationProof) IsEmpty() bool {
return b.BatchID == 0 &&
b.BlobIndex == 0 &&
b.BatchMetadata.IsEmpty() &&
len(b.InclusionProof) == 0 &&
len(b.QuorumIndices) == 0
}

// IsEmpty checks if BatchMetadata is effectively empty
func (bm BatchMetadata) IsEmpty() bool {
return bm.BatchHeader.IsEmpty() &&
len(bm.Fee) == 0 &&
bm.SignatoryRecordHash == [32]byte{} &&
bm.ConfirmationBlockNumber == 0 &&
len(bm.BatchHeaderHash) == 0
}

// IsEmpty checks if BatchHeader is effectively empty
func (bh BatchHeader) IsEmpty() bool {
return bh.BlobHeadersRoot == [32]byte{} &&
len(bh.QuorumNumbers) == 0 &&
len(bh.SignedStakeForQuorums) == 0 &&
bh.ReferenceBlockNumber == 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

keep meaning to refactor this type conversions - ty! 🙏🏻

@@ -35,11 +35,10 @@ func (d *readerForEigenDA) RecoverPayloadFromBatch(
preimageRecorder daprovider.PreimageRecorder,
validateSeqMsg bool,
) ([]byte, error) {
// offset sequencer message at 41
// offset sequencer message at 41
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can prolly do without these comments

Copy link
Author

Choose a reason for hiding this comment

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

cool! I removed the comments but added a constant SequencerMsgOffset to hold 41 for clearer usage

Copy link
Collaborator

@epociask epociask left a comment

Choose a reason for hiding this comment

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

LGTM

@hopeyen hopeyen merged commit 7b7729c into eigenda-v3.0.3 Aug 12, 2024
7 checks passed
@hopeyen hopeyen deleted the hope/custom-go-lint branch August 12, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants