Skip to content

Conversation

delbonis
Copy link
Contributor

@delbonis delbonis commented Oct 9, 2025

Description

This PR might be too large/complicated to merge without breaking other things. In such case, I can rework it.

Much of what was happening here was moving things that were defined in the primitives crate to instead be defined in a higher trait, which is then depended on by the primitives crate. There are a few places in the ledger-types PR (which introduced the new state access trait system) where we wanted to use something that was in the primitives crate, but we didn't actually want to depend on the whole primitives crate.

One interesting piece is this new identifiers crate that contains a bunch of common definitions that are used in very many places.

I used Claude pretty extensively to automate resolving import errors in downstream crates.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactor
  • New or updated tests
  • Dependency Update

Notes to Reviewers

Checklist

  • I have performed a self-review of my code.
  • I have commented my code where necessary.
  • I have updated the documentation if needed.
  • My changes do not introduce new warnings.
  • I have added (where necessary) tests that prove my changes are effective or that my feature works.
  • New and existing tests pass with my changes.
  • I have disclosed my use of AI
    in the body of this PR.

Related Issues

@delbonis delbonis requested a review from storopoli October 9, 2025 23:07
@delbonis delbonis marked this pull request as ready for review October 9, 2025 23:07
@delbonis delbonis requested review from a team as code owners October 9, 2025 23:07
@delbonis
Copy link
Contributor Author

delbonis commented Oct 9, 2025

Actually, this sucks, I think I fucked up the rebase I did to put it in its own branch instead of being in the other PR. There's changes in here like creating crates/ledger-types/src/lib.rs that don't make sense to be in this commit, idk why that's singled out. And I don't know how there's so many conflicts now.

All of this was because I wanted the AsmManifest type to be in a crate that was more appropriate and I didn't want the ledger-types crate to depend on the entire primitives crate in order to be able to use EpochCommitment. For example, we really don't need to have address parsing in the same crates as what's going to make its way into the OL STF proof.

Taking suggestions as to what to do here.

@storopoli
Copy link
Member

Needs rebase :/ let's try to get this in before #1086 since Dilli's out of the office today and he will need to rebase #1086 anyways.

storopoli

This comment was marked as outdated.

Copy link
Member

@storopoli storopoli left a comment

Choose a reason for hiding this comment

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

ACK 0225720

Is this related to https://alpenlabs.atlassian.net/browse/STR-1596 or https://alpenlabs.atlassian.net/browse/STR-1689? If yes, can you add that to the PR description body?

@delbonis
Copy link
Contributor Author

@storopoli This wasn't intended to be those tickets, although I guess it has a lot of overlap.

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