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

Add exact calculation of encoded transaction size #195

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

dailinsubjam
Copy link
Contributor

@dailinsubjam dailinsubjam commented Oct 24, 2024

Closes #169

This PR:

This PR does not:

Key places to review:

@dailinsubjam dailinsubjam marked this pull request as draft October 24, 2024 21:25
@coveralls
Copy link

coveralls commented Oct 24, 2024

Pull Request Test Coverage Report for Build 11509179894

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.05%) to 71.92%

Files with Coverage Reduction New Missed Lines %
crates/legacy/src/builder_state.rs 1 83.46%
crates/marketplace/src/builder_state.rs 1 73.71%
Totals Coverage Status
Change from base Build 11508863210: -0.05%
Covered Lines: 2382
Relevant Lines: 3312

💛 - Coveralls

@dailinsubjam dailinsubjam marked this pull request as ready for review October 25, 2024 00:03
Copy link
Member

Choose a reason for hiding this comment

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

Just make sure to replace the branch with the new HotShot tag.

Comment on lines +1455 to +1458
const NS_ID_BYTE_LEN: usize = 4;
const NS_OFFSET_BYTE_LEN: usize = 4;
const NUM_TXS_BYTE_LEN: usize = 4;
const TX_OFFSET_BYTE_LEN: usize = 4;
Copy link
Member

Choose a reason for hiding this comment

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

These look good to me, but we may want to remove the duplicate definitions in the sequencer repo, so that the sequencer will import these values from the builder repo, to avoid inconsistency.

Comment on lines +1555 to +1556
/// A const number on `max_tx_len` to be used consistently spanning all the tests
const TEST_MAX_TX_LEN: u64 = 20;
Copy link
Member

Choose a reason for hiding this comment

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

Double-checking how we get 20--is it length 4 + overhead 16? Can you add a comment here?

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.

Exact calculation of encoded transaction size
3 participants