Skip to content

Update remaining TODOs#50

Merged
JanKuczma merged 7 commits intomainfrom
jk-misc
Mar 4, 2026
Merged

Update remaining TODOs#50
JanKuczma merged 7 commits intomainfrom
jk-misc

Conversation

@JanKuczma
Copy link
Contributor

@JanKuczma JanKuczma commented Mar 3, 2026

Summary by CodeRabbit

  • Documentation

    • Updated security documentation by removing outdated PoC-specific limitation notes.
  • Chores

    • Removed internal documentation files.
    • Simplified build instructions in README.
  • Bug Fixes

    • Fixed account validation timing in migration wallet logic.

@coderabbitai
Copy link

coderabbitai bot commented Mar 3, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2ba3c350-4ac7-4dc9-b33e-66c636a3b387

📥 Commits

Reviewing files that changed from the base of the PR and between d2abc67 and 1b009f3.

📒 Files selected for processing (1)
  • noir/aztec-state-migration/src/mode_a/migration_data_event.nr

Walkthrough

This PR reduces public visibility of internal structures and methods across the Noir state migration library, removes proof-of-concept documentation files, updates documentation, reorders module declarations, and fixes a null-check ordering issue in TypeScript code.

Changes

Cohort / File(s) Summary
Documentation Cleanup
TODO_MODE_A.md, TODO_MODE_B.md, docs/security.md, README.md
Removed Mode A and Mode B PoC documentation files, deleted PoC limitation bullets from security documentation, and removed TypeScript build compilation reference from README.
Noir Visibility Restrictions (Mode A)
noir/aztec-state-migration/src/mode_a/migration_data_event.nr, noir/aztec-state-migration/src/mode_a/migration_note.nr
Restricted visibility of MigrationDataEvent<T> struct and its fields from public to pub(crate), and reduced MigrationNote struct fields from public to private.
Noir Module Reordering
noir/aztec-state-migration/src/mode_a/mod.nr
Reordered module declarations to move mod migration_lock and its re-export after the builder module declarations.
Noir Visibility Restrictions (Mode B)
noir/aztec-state-migration/src/mode_b/key_note_proof_data.nr, noir/aztec-state-migration/src/mode_b/non_nullification_proof_data.nr, noir/aztec-state-migration/src/mode_b/public_state_proof_data.nr
Restricted visibility of methods and struct fields from public to pub(crate) across proof data structures.
Noir Core Visibility Restrictions
noir/aztec-state-migration/src/note_proof_data.nr
Reduced visibility of struct fields (randomness, nonce, leaf_index, sibling_path) and methods (verify_note_inclusion, note_hash) to pub(crate) or private.
TypeScript Updates
ts/aztec-state-migration/wallet/migration-embedded-wallet.ts, ts/aztec-state-migration/mode-b/signature.ts
Moved account null-check earlier in the execution flow before constructor usage, and simplified imports to use only Fr and DOM_SEP__CLAIM_B.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Possibly related PRs

Suggested reviewers

  • DamianStraszak

Poem

🐰 The public doors now close with care,
Internal secrets tucked away fair,
Mode A and B docs take their flight,
While visibility dims to crate-private light,
A tidy refactor, precise and right! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Update remaining TODOs' is vague and generic, using non-descriptive terminology that does not clearly convey the specific changes made in the changeset. Consider a more specific title that describes the primary changes, such as 'Restrict internal API visibility in Noir and TypeScript migration modules' or 'Clean up PoC documentation and reduce public API surface'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch jk-misc

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@TODO.md`:
- Line 11: The release docs line under "Publish TS SDK package" currently uses
incorrect capitalization for GitHub; update the text that references the publish
workflow and the publish step (the "Publish TS SDK package" entry) so the
platform name is capitalized as "GitHub" everywhere in that line and any related
release documentation strings.

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 77181a0 and 4f8f147.

📒 Files selected for processing (5)
  • TODO.md
  • TODO_MODE_A.md
  • TODO_MODE_B.md
  • docs/security.md
  • ts/aztec-state-migration/wallet/migration-embedded-wallet.ts
💤 Files with no reviewable changes (3)
  • TODO_MODE_A.md
  • TODO_MODE_B.md
  • docs/security.md

@JanKuczma JanKuczma enabled auto-merge (squash) March 4, 2026 11:27
@JanKuczma JanKuczma merged commit 42a96f1 into main Mar 4, 2026
3 checks passed
@JanKuczma JanKuczma deleted the jk-misc branch March 4, 2026 11:35
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.

2 participants