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.

Sign up for GitHub

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

Jump to bottom

refactor: header-accumulator #16

Merged
merged 11 commits into from
Oct 23, 2024
Merged

refactor: header-accumulator #16

merged 11 commits into from
Oct 23, 2024
+251 −853

Conversation

gusinacio
Copy link
Contributor

No description provided.

@linear Linear
Copy link

linear bot commented Oct 21, 2024

BACK-69 Clean up `header-accumulator`

Clean up the header-accumulator crate. Do not add new features.

  • Any new public functions should be tested.
  • Any new public functions should be documented.

You are responsible for propagating any name changes you make.

@gusinacio gusinacio requested a review from pedrohba1 October 22, 2024 09:41
@gusinacio gusinacio force-pushed the gustavo/back-69-clean-up-header-accumulator branch from 23a0794 to b61f026 Compare October 22, 2024 09:45
@gusinacio gusinacio marked this pull request as ready for review October 22, 2024 10:11
suchapalaver
suchapalaver requested changes Oct 22, 2024
View reviewed changes
crates/header-accumulator/src/errors.rs Outdated Show resolved Hide resolved
crates/header-accumulator/src/errors.rs Outdated Show resolved Hide resolved
crates/header-accumulator/src/errors.rs Outdated Show resolved Hide resolved
suchapalaver
suchapalaver requested changes Oct 22, 2024
View reviewed changes
crates/header-accumulator/src/errors.rs Outdated Show resolved Hide resolved
crates/header-accumulator/src/errors.rs Outdated Show resolved Hide resolved
anirudh2
anirudh2 requested changes Oct 22, 2024
View reviewed changes
Copy link
Member

@anirudh2 anirudh2 left a comment

Choose a reason for hiding this comment

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

Gustavo, why is it that half the time I review your PRs, I open them to see 50+ files changed hahaha?

Generally, this is much, much improved. Thanks a ton!

I added a few suggestions. I do want to make another pass once you've addressed all the change requests.

crates/header-accumulator/src/errors.rs Outdated Show resolved Hide resolved
crates/header-accumulator/src/errors.rs Outdated Show resolved Hide resolved
crates/header-accumulator/src/inclusion_proof.rs Show resolved Hide resolved
suchapalaver
suchapalaver previously approved these changes Oct 22, 2024
View reviewed changes
suchapalaver
suchapalaver previously approved these changes Oct 23, 2024
View reviewed changes
anirudh2
anirudh2 previously approved these changes Oct 23, 2024
View reviewed changes
Copy link
Member

@anirudh2 anirudh2 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all of our feedback! Your refactor has much, much improved this code!

pedrohba1
pedrohba1 previously approved these changes Oct 23, 2024
View reviewed changes
@gusinacio
refactor: header-accumulator
924aefa 
Signed-off-by: Gustavo Inacio <[email protected]>
@gusinacio
chore: update tree_hash version
889e191 
Signed-off-by: Gustavo Inacio <[email protected]>
@gusinacio
refactor: cleanup header accumulator
e22b411 
Cleanup errors
Added more checks to epoch creation

Signed-off-by: Gustavo Inacio <[email protected]>
@gusinacio
fix: flat-head usage of header accumulator
99db5bd 
Signed-off-by: Gustavo Inacio <[email protected]>
@gusinacio
test: use validate era instead of eras
0d753ec 
Signed-off-by: Gustavo Inacio <[email protected]>
@gusinacio
test: move compression test to decoder
c63a700 
Signed-off-by: Gustavo Inacio <[email protected]>
@gusinacio
feat: add more information to errors
3694897 
Signed-off-by: Gustavo Inacio <[email protected]>
@gusinacio
docs: document all public functions
9b08f72 
chore: update imports and remove some public functions
Signed-off-by: Gustavo Inacio <[email protected]>
@gusinacio
docs: add missing line
9f9230b 
Signed-off-by: Gustavo Inacio <[email protected]>
@gusinacio
style: make it more rusty
a51b29f 
Signed-off-by: Gustavo Inacio <[email protected]>
@gusinacio gusinacio dismissed stale reviews from pedrohba1 and anirudh2 via a51b29f October 23, 2024 14:52
@gusinacio gusinacio force-pushed the gustavo/back-69-clean-up-header-accumulator branch from b771356 to a51b29f Compare October 23, 2024 14:52
@gusinacio gusinacio requested a review from pedrohba1 October 23, 2024 14:52
@gusinacio gusinacio enabled auto-merge (rebase) October 23, 2024 14:52
@gusinacio
chore: fix rebase
b148f5f 
Signed-off-by: Gustavo Inacio <[email protected]>
@gusinacio gusinacio merged commit d7b2eb1 into main Oct 23, 2024
6 of 8 checks passed
@gusinacio gusinacio deleted the gustavo/back-69-clean-up-header-accumulator branch October 23, 2024 15:04
suchapalaver added a commit that referenced this pull request Nov 28, 2024
@suchapalaver
Merge pull request #16 from semiotic-ai/joseph/tru-229-rename-beacon-…
b371c9a 
…state-proofs-to-forrestrie

chore: rename project to forrestrie
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedrohba1 pedrohba1 pedrohba1 approved these changes

@suchapalaver suchapalaver suchapalaver left review comments

@anirudh2 anirudh2 anirudh2 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gusinacio @anirudh2 @pedrohba1 @suchapalaver