Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(core): move address logic to crate #1802

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

SuperFluffy
Copy link
Member

@SuperFluffy SuperFluffy commented Nov 12, 2024

Summary

Breaks out address related logic in astria_core::primitive::v1 to astria-core-address

Background

One needs to import the entire kitchensink that is astria-core even if one only wants to use a fraction of its types. That leads to extremely long compilation times because all of its dependencies need to be compiled.

This patch is the start to breaking out parts of astria-core into their own free standing crates, which astria-core then reexports.

Changes

  • Move address related types out of astria_core::primitive::v1 into astria-core-address
  • Reexport all moved types under the same module (aliasing, where necessary)
  • Remove the serde Serialize and Deserialize impls on the domain type Address (breaking because one needs to explicitly construct protobuf/wire type Address; not breaking on the wire, json format stays the same)
  • Implement the Protobuf ("domain type") trait for Address, removing its inherent constructors and methods to move from/to raw protobuf types ("wire types"); decouples astria-core-address from astria-core (specifically, the generated protobuf types) (breaking for consumers because they now need to import the Protobuf trait)
  • Create a thin crate astria-core-consts that only contains the const ADDRESS_LENGTH to allow decoupling of address and crypto logic (addressed in a separate patch)
  • Removed AddressBuilder::with_iter from the public interface (only used inside the crate and not outside) (breaking for consumers of that method)

Testing

Tests were shuffled around but not removed. Snapshot tests stay in place in astria core for now (because they are required for wire/protobuf types).

Changelogs

Changelogs updated.

Breaking Changelist

  • These changes leave all services untouched.
  • If astria-core gave stability guarantees, this would be a breaking change for consumers of astria-core because some inherent methods were removed from its public API.

Related Issues

Part of #1798

@github-actions github-actions bot added the sequencer pertaining to the astria-sequencer crate label Nov 12, 2024
@SuperFluffy SuperFluffy force-pushed the ENG-1003/address-to-crate branch 3 times, most recently from 48eb8f4 to d387f5f Compare November 12, 2024 08:50
@SuperFluffy SuperFluffy marked this pull request as ready for review November 12, 2024 08:53
@SuperFluffy SuperFluffy requested a review from a team as a code owner November 12, 2024 08:53
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this new crate is shared with #1800

@SuperFluffy SuperFluffy added the core pertaining to the astria-core crate label Nov 12, 2024
@SuperFluffy
Copy link
Member Author

Rebased and squashed on top of recent main to incorporate the addition of the consts crate. Also pointed the consts crate to its initial release.

@SuperFluffy SuperFluffy added this pull request to the merge queue Nov 20, 2024
@SuperFluffy SuperFluffy removed this pull request from the merge queue due to a manual request Nov 20, 2024
@SuperFluffy SuperFluffy added this pull request to the merge queue Nov 20, 2024
Merged via the queue into main with commit 3e85e8d Nov 20, 2024
46 checks passed
@SuperFluffy SuperFluffy deleted the ENG-1003/address-to-crate branch November 20, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core pertaining to the astria-core crate sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants