refactor: generate internal Header.encodeRLP() for override
#86
Conversation
ARR4N
requested review from
a team,
darioush,
ceyonur and
michaelkaplan13
and removed request for
a team
| - name: Set up Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: 1.21.4 |
There was a problem hiding this comment.
nit shouldn't we stay on 1.20 given that's what in the go.mod? 🤔
There was a problem hiding this comment.
Both the go.mod and the go.yml inherit their versions from upstream v1.13.14 for now. I'm happy to look at changing them but think we should consider it more thoroughly first.
If you want to send a PR that uses a matrix strategy for multiple versions I think that will be the best of both worlds. Please try to keep the pull_request to main workflow trigger fast / limited to one version and leave the multi-version tests to main push + everything to release/* (I'm not 100% sure this is possible, but it would be nice for PRs to not have to wait half an hour).
There was a problem hiding this comment.
Why do we want to support/check it works on versions above the one in go.mod though? When we later sync with geth, we can just bump all CI versions to the geth go.mod version (and CI may or may not fail). I'm creating (slowly) an issue to list CI checks we could add for post-syncing, for example keeping go-version of CI jobs in sync with go.mod (perhaps dependabot does it). Maybe a more appropriate PR would be to pin all go-version of jobs to 1.20? 🤔
| - name: Run `go generate` | ||
| run: go list ./... | grep -Pv "${EXCLUDE_REGEX}" | xargs go generate; |
There was a problem hiding this comment.
You can use the -skip flag I think:
| - name: Run `go generate` | |
| run: go list ./... | grep -Pv "${EXCLUDE_REGEX}" | xargs go generate; | |
| - run: go generate -skip "${EXCLUDE_REGEX}" ./... |
There was a problem hiding this comment.
Thanks, TIL about -skip. This doesn't work for me locally though (I suspect because the regex needs to change):
$ go generate -skip 'ava-labs/libevm/(accounts/usbwallet/trezor)$' ./...
/usr/local/include: warning: directory does not exist.
protoc-gen-go: no such flag -import_path
--go_out: protoc-gen-go: Plugin failed with status code 1.
accounts/usbwallet/trezor/trezor.go:45: running "protoc": exit status 1If you add the matrix strategy and switching to -skip is simple then please include it, but I think it should be timeboxed as it's low yield.
There was a problem hiding this comment.
My apologies, similarly to -run, it's for the source text in the //go:generate statement, not the file path. Better to keep it as it is!
## Why this should be merged Replaces #86. That approach couldn't be replicated for generated JSON marshalling, and this once also reduces the upstream delta. ## How this works AST manipulation of a specified file, which finds specified methods and modifies their names to be unexported. ## How this was tested Inspection of the output (`core/types/gen_header_rlp.go` remains unchanged).
Why this should be merged
This is a precursor to being able to override
types.HeaderRLP {en,de}coding. As there is already aHeader.EncodeRLP()method we either have to modify the generated code or rename the generated method—this PR does the latter.How this works
The
rlpgen -internal_methodsflag changes the generated methods fromEncodeRLP()andDecodeRLP()toencodeRLP()anddecodeRLP(), respectively. A new CI job checks that generated code is up to date. We can then implement our ownHeader.EncodeRLP()that either overrides or falls back on the original.It appears that
core/gen_genesis.gowas out of date but only because of formatting.How this was tested
I deliberately excluded the change to
core/types/gen_header_rlp.goto confirm that the new workflow detects the change and fails. The actual change can be inspected via the code diff.