Skip to content

Various cleanups#8594

Merged
turbolent merged 3 commits into
masterfrom
bastian/extract-cadence-vm-unrelated
Jul 13, 2026
Merged

Various cleanups#8594
turbolent merged 3 commits into
masterfrom
bastian/extract-cadence-vm-unrelated

Conversation

@turbolent

@turbolent turbolent commented Jun 30, 2026

Copy link
Copy Markdown
Member
  • Extract improvements from feature/cadence-vm unrelated to Cadence VM work
  • Simplify/optimize address conversion

Summary by CodeRabbit

  • Bug Fixes

    • Improved how account/recipient addresses are encoded when constructing transactions and calling system contracts, reducing the risk of encoding-related issues.
    • Corrected and renamed several configuration option helpers for contract deployment/removal restrictions and authorization check toggles (same behavior, clearer names).
  • Style

    • Reformatted CI and verification logic for consistency without changing behavior.
  • Tests

    • Updated integration test helpers and expectations to align with the updated address handling and option naming.

@turbolent turbolent requested a review from a team as a code owner June 30, 2026 23:25
@github-actions

Copy link
Copy Markdown
Contributor

Dependency Review

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

Scanned Files

None

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR reformats the Docker build workflow, renames fees and FVM option helpers, reformats verifier calls, and replaces byte-based Cadence address conversions with direct address construction across FVM, benchmark, and integration code.

Changes

CI Workflow Formatting

Layer / File(s) Summary
Docker build workflow formatting
.github/workflows/ci.yml
Reformats the Docker build step and adjusts spacing near Docker image caching without changing commands or environment values.

Go Code Fixes and Cadence Updates

Layer / File(s) Summary
Transaction fees flag rename
cmd/util/cmd/verify_execution_result/cmd.go
Renames flagtransactionFeesDisabled to flagTransactionFeesDisabled in its declaration, flag binding, and verifier call sites.
Verifier function formatting
engine/verification/verifier/verifiers.go
Reformats the VerifyRange parameter list and makeVerifier call without changing behavior.
FVM context options and authorizers
fvm/context.go, fvm/blueprints/contracts.go, fvm/accounts_test.go
Renames three option helpers, fixes a comment typo, uses direct Cadence address construction for authorizers, and reformats a test chain.
Direct Cadence address construction
fvm/environment/system_contracts.go, fvm/environment/system_contracts_test.go, integration/benchmark/scripts/scripts.go, integration/tests/access/cohort3/*
Replaces byte-based address conversions with direct Cadence address construction across system contracts and transaction helpers; the system contract test passes nil for absent arguments.

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

Suggested reviewers: janezpodhostnik, ramtinms, zhangchiqing

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is too vague and doesn't communicate the main changes in the pull request. Use a specific title summarizing the key change, such as address conversion cleanup and exported helper renames.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bastian/extract-cadence-vm-unrelated

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

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

@codecov-commenter

codecov-commenter commented Jun 30, 2026

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
fvm/context.go (1)

245-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Keep deprecated aliases for the renamed option helpers. Renaming these exported functions breaks downstream callers; add thin deprecated wrappers unless this is meant to be a breaking API change.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@fvm/context.go` around lines 245 - 250, The exported option helper
WithContractRemovalRestricted was renamed and now breaks downstream callers;
keep the new helper but add a thin deprecated wrapper under the old name so
existing users still compile. Update the relevant option helpers in context.go
by preserving the current implementation and introducing the deprecated alias
function that forwards to it, matching the pattern used by other renamed APIs if
present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@fvm/context.go`:
- Around line 245-250: The exported option helper WithContractRemovalRestricted
was renamed and now breaks downstream callers; keep the new helper but add a
thin deprecated wrapper under the old name so existing users still compile.
Update the relevant option helpers in context.go by preserving the current
implementation and introducing the deprecated alias function that forwards to
it, matching the pattern used by other renamed APIs if present.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 68f29d81-d58b-4596-b847-5ac1342776a0

📥 Commits

Reviewing files that changed from the base of the PR and between 4c61355 and 91658e4.

📒 Files selected for processing (11)
  • .github/workflows/ci.yml
  • cmd/util/cmd/verify_execution_result/cmd.go
  • engine/verification/verifier/verifiers.go
  • fvm/accounts_test.go
  • fvm/blueprints/contracts.go
  • fvm/context.go
  • fvm/environment/system_contracts.go
  • fvm/environment/system_contracts_test.go
  • integration/benchmark/scripts/scripts.go
  • integration/tests/access/cohort3/extended_indexing_test.go
  • integration/tests/access/cohort3/extended_indexing_transfers_test.go

@turbolent turbolent enabled auto-merge July 13, 2026 16:46

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
fvm/environment/system_contracts.go (1)

123-123: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add regression coverage for the direct address conversions.

The PR context reports 0% patch coverage for this file. Add focused tests proving that direct cadence.Address(...) conversion preserves addresses, including leading-zero addresses, across fee, balance, and storage-capacity calls.

Also applies to: 160-160, 184-184, 210-210, 232-232, 245-245, 264-264

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@fvm/environment/system_contracts.go` at line 123, Add focused regression
tests for the direct cadence.Address conversions used by the fee, balance, and
storage-capacity contract calls. Verify normal and leading-zero addresses are
preserved through each conversion path, covering the payer and all related
address arguments identified around the affected conversion sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@fvm/environment/system_contracts.go`:
- Line 123: Add focused regression tests for the direct cadence.Address
conversions used by the fee, balance, and storage-capacity contract calls.
Verify normal and leading-zero addresses are preserved through each conversion
path, covering the payer and all related address arguments identified around the
affected conversion sites.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a5948abe-c429-4163-813a-4fc0efa5ac47

📥 Commits

Reviewing files that changed from the base of the PR and between 91658e4 and 45180a7.

📒 Files selected for processing (2)
  • .github/workflows/ci.yml
  • fvm/environment/system_contracts.go

@turbolent turbolent added this pull request to the merge queue Jul 13, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 13, 2026
@turbolent turbolent added this pull request to the merge queue Jul 13, 2026
Merged via the queue into master with commit d7463b7 Jul 13, 2026
61 checks passed
@turbolent turbolent deleted the bastian/extract-cadence-vm-unrelated branch July 13, 2026 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants