Remove obsolete TODOs in fvm and apply small fixes

[janezpodhostnik]

Reviewed the TODO comments in the fvm package and investigated each one
against the current code. This PR removes 5 obsolete TODOs and resolves
3 more with small fixes.

Removed (obsolete)

• blueprints/contracts.go: "get rid of authorizers". Under Cadence 1.0 the
  authorizer is required: the deploy template takes an
  auth(AddContract) &Account signer, obtainable only from a transaction
  authorizer.
• evm/emulator/state/collection.go: "expose SlabID.Address() in atree".
  atree v0.16.1 exports it; replaced the AddressAsUint64 workaround with
  slabID.Address().
• evm/types/precompiled.go: "skip encoding inputs in future versions".
  Already done: encoding v2 (current) dropped the input fields.
• fvm_test.go: "this should be sc.EVM.Address not found there???". The
  field exists as sc.EVMContract; switched to it (value-identical by
  design, EVMContract.Address == FlowServiceAccount.Address).
• evm/handler/handler_test.go: "add unhappy-case emulator tests". They
  exist: withdraw/deposit unhappy cases with injected emulator failures.

Fixed

• errors/base.go: NewInvalidArgumentErrorf doc now lists the actual
  failure cases instead of a TODO.
• runtime/reusable_cadence_runtime.go: unexported SwappableEnvironment
  (now swappableEnvironment), the last exported type flagged by the TODO.
  All uses were in-package.
• transactionPayerBalanceChecker_test.go: filled in the fixture location
  and qualified identifier to match the real FlowFees
  VerifyPayerBalanceResult type.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

Summary by CodeRabbit

• Documentation

  • Clarified when invalid argument errors can occur, including arguments that cannot be decoded as JSON.

• Refactor

  • Improved internal runtime encapsulation and collection address handling without changing externally visible behavior.

• Tests

  • Updated contract deployment and payer balance test setup to use more accurate contract and address information.

• Maintenance

  • Removed outdated implementation notes and TODO comments.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: f19cdc08-2ce5-4bc9-9e30-aefb4728a19e

📥 Commits

Reviewing files that changed from the base of the PR and between 37e6f36 and 4c4fba5.

📒 Files selected for processing (10)

• fvm/blueprints/contracts.go
• fvm/errors/base.go
• fvm/evm/emulator/state/collection.go
• fvm/evm/handler/handler.go
• fvm/evm/handler/handler_test.go
• fvm/evm/types/precompiled.go
• fvm/fvm_test.go
• fvm/runtime/cadence_function_declarations.go
• fvm/runtime/reusable_cadence_runtime.go
• fvm/transactionPayerBalanceChecker_test.go

💤 Files with no reviewable changes (3)

• fvm/blueprints/contracts.go
• fvm/evm/types/precompiled.go
• fvm/evm/handler/handler_test.go

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Walkthrough

The changes make the reusable runtime environment package-private, update related references, correct address-based test fixtures, simplify slab address handling, and remove or update obsolete comments.

Changes

Runtime encapsulation and cleanup

Layer / File(s)	Summary
Make runtime environment APIs package-private fvm/runtime/..., fvm/evm/handler/handler.go	The reusable runtime now uses swappableEnvironment and registerOnSwapCallback. Related EVM cache cleanup and comments use the updated names.
Align address handling and test fixtures fvm/evm/emulator/state/collection.go, fvm/fvm_test.go, fvm/transactionPayerBalanceChecker_test.go	Collection lookup uses slabID.Address(). Tests use the configured EVM contract address and a concrete FlowFees result location.
Update comments and remove obsolete TODOs fvm/errors/base.go, fvm/blueprints/contracts.go, fvm/evm/handler/handler_test.go, fvm/evm/types/precompiled.go	The invalid-argument documentation lists JSON decoding. Obsolete TODO comments are removed.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: turbolent

Merge Risk: ⚪ Minimal · up to 4c4fb

This cleanup makes the reusable runtime private and corrects address-based code and fixtures without changing the intended runtime behavior; no actionable current-head risk is established.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 7 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the pull request. It identifies obsolete TODO removal and the related small fixes across the FVM package.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch janezp/fvm-todo-cleanup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[zhangchiqing]

No concerns from this review. No execution-behavior changes: comment/TODO removals, one value-identical address derivation, one type unexport, and test-fixture metadata — no HCU implications, no error-message changes.

Verified (all correct):

• fvm/evm/emulator/state/collection.go:55 — atree v0.16.1 SlabID.Address() returns id.address directly (atree slab_id.go:114), byte-identical to the removed PutUint64(address[:], slabID.AddressAsUint64()) roundtrip. encoding/binary import still used elsewhere in the file.
• fvm/fvm_test.go:3350 — sc.EVMContract.Address == sc.FlowServiceAccount.Address by construction: both ContractNameEVM and ContractNameServiceAccount map to serviceAddressFunc in fvm/systemcontracts/system_contracts.go, so the switch is value-identical on every chain.
• fvm/runtime/reusable_cadence_runtime.go — repo-wide grep confirms zero references to SwappableEnvironment/RegisterOnSwapCallback outside fvm/runtime (including the integration module), and no exported types remain in the file, so the removed TODO is fully resolved.
• fvm/errors/base.go:27 — the new doc line matches the sole production callsite (fvm/environment/programs.go:214: "argument is not json decodable"). Doc-only; error message (execution-result-visible) untouched.
• fvm/evm/types/precompiled.go — current encoding v2 (AggregatedPrecompiledCallsEncodingVersion = 2) has no input fields; only legacy precompiledCallsV1 carried Input. TODO correctly obsolete.
• fvm/evm/handler/handler_test.go — unhappy-case emulator tests with injected TestEmulator failures exist (withdraw L459, deposit L534, transaction run L836, batch run L1016).
• fvm/blueprints/contracts.go — deployContractTransactionTemplate.cdc uses prepare(signer: auth(AddContract) &Account), so the authorizer is indeed required under Cadence 1.0.

Build plus targeted tests pass: go build ./fvm/... and go test on fvm (TestTransactionPayerBalanceChecker), fvm/evm/handler (TestHandler_COA), fvm/evm/emulator/state, fvm/runtime.

Nit (1)

• fvm/transactionPayerBalanceChecker_test.go:22-23 — the fixture's qualified identifier hardcodes address 0x1, which is not the FlowFees deployment address on any chain. Harmless because DecodeVerifyPayerBalanceResult only extracts fields by name and never compares the type ID, but "mirrors the FlowFees contract" would read truer with a real chain address (e.g. the emulator service account).