Skip to content

starknet_committer: name TrieReadTask lifetimes and document them#14239

Merged
yoavGrs merged 1 commit into
mainfrom
yoav/triereadtask-lifetime-names
May 31, 2026
Merged

starknet_committer: name TrieReadTask lifetimes and document them#14239
yoavGrs merged 1 commit into
mainfrom
yoav/triereadtask-lifetime-names

Conversation

@yoavGrs
Copy link
Copy Markdown
Contributor

@yoavGrs yoavGrs commented May 28, 2026

The struct now uses 'indices / 'updates / 'storage instead of 'a / 'u /
's, and the doc comment explains why two non-storage lifetimes are
needed: unifying them would make the returned OriginalSkeletonForest
appear to borrow the storage update maps too, preventing the compute
phase from moving those maps by value.

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

@reviewable-StarkWare
Copy link
Copy Markdown

This change is Reviewable

Copy link
Copy Markdown
Contributor Author

yoavGrs commented May 28, 2026

@yoavGrs yoavGrs self-assigned this May 28, 2026
@yoavGrs yoavGrs removed the request for review from ArielElp May 28, 2026 09:15
@yoavGrs yoavGrs marked this pull request as ready for review May 28, 2026 09:15
@cursor
Copy link
Copy Markdown

cursor Bot commented May 28, 2026

PR Summary

Low Risk
Comment and lifetime-parameter renames only; no runtime or commit-path behavior changes.

Overview
Renames opaque lifetime parameters on TrieReadTask and its StorageTask impls to 'indices, 'updates, and 'storage, and expands the struct doc to spell out why 'indices and 'updates must stay separate (so OriginalSkeletonForest does not appear to borrow storage update maps and block moving them into the compute phase in commit_block).

No logic or API behavior changes—only naming and comments in trie_traversal.rs.

Reviewed by Cursor Bugbot for commit bd2416b. Bugbot is set up for automated code reviews on this repo. Configure here.

Copy link
Copy Markdown
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

@dorimedini-starkware reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on yoavGrs).

Copy link
Copy Markdown
Contributor

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

@ArielElp reviewed 1 file and all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on yoavGrs).

@graphite-app graphite-app Bot changed the base branch from yoav/skeleton-updates-from-actual to graphite-base/14239 May 28, 2026 12:52
@yoavGrs yoavGrs force-pushed the graphite-base/14239 branch from 98f94f2 to 399a721 Compare May 29, 2026 10:00
@yoavGrs yoavGrs force-pushed the yoav/triereadtask-lifetime-names branch from db78a3c to 9597670 Compare May 29, 2026 10:00
@graphite-app graphite-app Bot changed the base branch from graphite-base/14239 to main May 29, 2026 10:01
@graphite-app
Copy link
Copy Markdown

graphite-app Bot commented May 29, 2026

Merge activity

  • May 29, 10:01 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.

@yoavGrs yoavGrs force-pushed the yoav/triereadtask-lifetime-names branch from 9597670 to c9b7fe9 Compare May 31, 2026 08:37
Copy link
Copy Markdown
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on yoavGrs).

The struct now uses 'indices / 'updates / 'storage instead of 'a / 'u /
's, and the doc comment explains why two non-storage lifetimes are
needed: unifying them would make the returned OriginalSkeletonForest
appear to borrow the storage update maps too, preventing the compute
phase from moving those maps by value.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@yoavGrs yoavGrs force-pushed the yoav/triereadtask-lifetime-names branch from c9b7fe9 to bd2416b Compare May 31, 2026 12:46
@yoavGrs yoavGrs added this pull request to the merge queue May 31, 2026
Merged via the queue into main with commit 1dc19d2 May 31, 2026
28 checks passed
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