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

chore: Change EventInsertable to store the parsed Event object instead of its raw blocks #483

Closed
wants to merge 5 commits into from

Conversation

stbrody
Copy link
Collaborator

@stbrody stbrody commented Aug 15, 2024

builds on #481

Removes the EventInsertableBody type and pushes its fields up into the parent EventInsertable
}
/// Underlying bytes that make up the event
pub async fn get_raw_blocks(&self) -> Result<Vec<EventBlockRaw>> {
let car = self.event.encode_car().await.map_err(Error::new_app)?;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hate this. Encoding it to a CAR just to re-encode it to the raw blocks is doing two passes over the data for no reason. I tried to add a method to unvalidated::Event that worked just like encode_car() but would instead return a Vec<EventBlockRaw> so we could get what we need in a single pass. The problem is that the EventBlockRaw type lives in the ceramic-store crate while the unvalidated::Event type lives in the ceramic-event crate, but the store crate depends on the event crate, so I can't make the event crate depend on the store crate without introducing a circular dependency.

Would love any suggestions on how to clean this up.

@stbrody
Copy link
Collaborator Author

stbrody commented Aug 15, 2024

This is passing all tests except the migration tests, I might need a hand to figure out what's up with the migration tests.

@stbrody
Copy link
Collaborator Author

stbrody commented Aug 16, 2024

Okay I figured out a part of what's wrong with the migration tests - there are some event CAR files being used in that test that don't round-trip through unvalidated::Event. Pushed a change that demonstrates that. Now the question is: is this a bug in our serialization/deserialization logic? Or is the migration test using invalid CAR files?

@nathanielc

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.

1 participant