Skip to content

test(platform-wallet): pass on_built in the fixed-amount asset-lock test helper - #5732

Closed
shumkov wants to merge 1 commit into
v5.1-devfrom
shumkov/v51-fix-asset-lock-test-args
Closed

shumkov wants to merge 1 commit into
v5.1-devfrom
shumkov/v51-fix-asset-lock-test-args

Conversation

@shumkov

@shumkov shumkov commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Issue being fixed or feature implemented

v5.1-dev no longer compiles the platform-wallet test target, so the push run's all-targets clippy fails with E0061 (latest: run 41594542638 on 40039c9244). Two PRs merged back to back each passed on their own base:

What was done?

Pass None for on_built in that helper, as the other test call sites do. Test code only.

How Has This Been Tested?

  • cargo clippy -p platform-wallet --all-targets --all-features --locked -- --no-deps -D warnings: fails with E0061 (2 errors) without this change, passes with it.
  • cargo test -p platform-wallet --lib --all-features asset_lock::build: 48 passed.

Breaking Changes

None.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

PR Hygiene · 9fd84ec

  • Bots — thepastaclaw ✓
  • Self-review — post /self-reviewed
  • Reviewer requests paused — your 5 review slots are occupied; this PR is excluded from reviewers' queues. Required approvals still count without a slot.
  • Build running
  • Approvals
    • rs-platform-wallet (packages/rs-platform-wallet/src/wallet/asset_lock/build.rs) — HashEngineering or ZocoLini or llbartekll or romchornyi

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

Summary by CodeRabbit

  • Chores
    • No end-user-visible changes in this update.

…est helper

#5712 added broadcast_exact_registration_lock, which calls
broadcast_funded_asset_lock_with_operation with seven arguments. #5476,
merged right after it, added the eighth (on_built). Each PR passed on its
own base, but together they leave the platform-wallet test target
uncompilable, so v5.1-dev's all-targets clippy fails with E0061.

Pass None, as the other test call sites do.

Test would have caught this in CI: cargo clippy -p platform-wallet
--all-targets --all-features fails with E0061 before this change and
passes after; the 48 asset_lock::build tests pass.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@shumkov shumkov added this to the v5.1.0 milestone Nov 3, 2026
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The registration-lock helper now passes None for the optional built-lock observer when it calls broadcast_funded_asset_lock_with_operation.

Changes: Asset-lock broadcast call

Layer / File(s) Summary
Set the optional observer argument
packages/rs-platform-wallet/src/wallet/asset_lock/build.rs
The helper passes None as the built-lock observer argument to the operation-based broadcast call.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Bug fix

Possibly related PRs

Suggested reviewers: romchornyi, hashengineering

Merge Risk: Minimal · up to 9fd84

The change supplies the missing optional callback argument without changing production code. No actionable merge-blocking risk was identified; mergeable subject to normal checks.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly identifies the main change: passing the on_built argument in the fixed-amount asset-lock test helper.
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
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.
Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.

Finishing Touches
Generate docstrings
  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 5ec9631 and 9fd84ec.

Files selected for processing: 1
  • packages/rs-platform-wallet/src/wallet/asset_lock/build.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added the waiting-bots Waiting for the review bots to report on this head label Nov 3, 2026
@thepastaclaw

thepastaclaw commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Final review complete — no blockers (commit 9fd84ec) · triage: low

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Final validation — Phase 1 + Phase 2

Verified the one-line change at head 9fd84ec: passing None correctly supplies the optional on_built argument without bypassing operation tracking or persistence, and the change is confined to test code. No actionable defects were found. Validation was static only; current GitHub checks show wallet runner selection still pending and no failed code checks, so the supplied claim that wallet CI has passed is not established.

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: low by gpt-6.1-sol (effort low) — The diff adds a single missing None argument to a Rust test helper, restoring compatibility with the updated method signature without changing production behavior.
  • Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer, glm-5.3-flash — rust-quality (completed, effort high); agent phase1-reviewer
  • Phase 1 model: glm-5.3-flash — zai quota: 5h 77% left, weekly 83% left; passed over gemini-3.8-flash-high (antigravity quota lookup failed)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort medium); agent phase2-reviewer

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Nov 3, 2026
@shumkov

shumkov commented Nov 3, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as a duplicate: #5734 fixed the same E0061 in the same test helper and is merged into v5.1-dev.

@shumkov shumkov closed this Nov 3, 2026
@shumkov
shumkov deleted the shumkov/v51-fix-asset-lock-test-args branch November 3, 2026 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-self-review Waiting for the author to post /self-reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants