refactor: address review comments on legacy adapter - #2
Draft
JordiParraCrespo wants to merge 1 commit into
Draft
Conversation
Rename for clarity, per review feedback on PR xrplevm#12: - const V9 -> CosmosEVMUpgradeName - LegacyAdapter.v9Height field -> cosmosEvmHeight - UnmarshalLegacyParams -> UnmarshalParams (unify with GetCodeHash / UnmarshalFeeMarketParams naming) - test helper loadedAdapter -> newLoadedTestLegacyAdapter - drop the "->" arrows from doc comments Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R63pArhK88pBunBwBHgzn9
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the review feedback left on xrplevm/evm#12 (
fix: decode pre-v9 state via height-dispatched legacy adapter). Pure renames / comment cleanups — no behavior change.This PR is based on the
fix/legacy-state-readsbranch (a copy of PR xrplevm#12) inside this fork, so the diff shown here is only the review fixes.Changes (per review comment)
adapter.go— generic naming (xrplevm#3/xrplevm#4)const V9→const CosmosEVMUpgradeName(value"v9.0.0"unchanged); doc comment updatedadapter.go:40(#2)v9Height→cosmosEvmHeightparams.go:20(xrplevm#6)UnmarshalLegacyParams→UnmarshalParams(unify withGetCodeHash/UnmarshalFeeMarketParams, which carry noLegacyprefix)params.go:61(xrplevm#7)UnmarshalParamsandUnmarshalFeeMarketParamsparams.go:26+ package doc (xrplevm#8)→arrows from commentsadapter_test.go:19(xrplevm#5)loadedAdapter→newLoadedTestLegacyAdapterCall sites updated:
x/vm/keeper/params.go,legacy/legacytestutil, and thelegacytests.On the generic-version comment (xrplevm#3/xrplevm#4)
Kept the upgrade name as a single, properly-named exported constant (
CosmosEVMUpgradeName) rather than a full pluggable/configurable redesign, per the scope agreed for this follow-up.Not changed here —
evmd/app.goLoad() question (#1)That comment is a question about whether the
legacyAdapter.Load()inLoadHeightis redundant given the load in the constructor. Left untouched pending author discussion (analysis provided separately).Verification
gofmt -l legacy x/vm/keeper/params.go— cleango build ./...— passes (root module andevmd/)go test ./legacy/...— passes🤖 Generated with Claude Code
https://claude.ai/code/session_01R63pArhK88pBunBwBHgzn9
Generated by Claude Code