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

Replace read_compactsize and write_compactsize with CompactSizeMessage #3014

Merged
merged 4 commits into from
Nov 5, 2021
Merged

Replace read_compactsize and write_compactsize with CompactSizeMessage #3014

merged 4 commits into from
Nov 5, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Nov 4, 2021 •
edited by oxarbitrage
Loading

Motivation

To implement addrv2 in #2681, we need a CompactSize type that can be larger than MAX_PROTOCOL_MESSAGE_LEN.

This is the second part of that change, which does not block the first part of ticket #2681.
(But it blocks completing that ticket.)

Solution

  • Replace read_compactsize and write_compactsize with CompactSizeMessage
  • Add tests for CompactSize64
  • Add compact size range and conversion tests

Closes #2173.

Review

@oxarbitrage can review this PR.

This is a medium-priority refactor, because parts of #2681 conflict with this change.

Reviewer Checklist

  • Refactor makes sense
  • Existing tests pass

Follow Up Work

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement C-cleanup Category: This is a cleanup P-Medium labels Nov 4, 2021
@teor2345 teor2345 requested a review from oxarbitrage November 4, 2021 04:53
@teor2345 teor2345 self-assigned this Nov 4, 2021
@teor2345 teor2345 linked an issue Nov 4, 2021 that may be closed by this pull request
Allow CompactSize to be used for non-length fields (addrv2 and ZSEs) #2173
Closed
Base automatically changed from compact-size-unlimited to main November 4, 2021 15:57
@teor2345 teor2345 force-pushed the compact-size-conversion branch from dd01bb5 to 4cb076a Compare November 4, 2021 22:50
@teor2345 teor2345 enabled auto-merge (squash) November 4, 2021 22:56
@teor2345 teor2345 mentioned this pull request Nov 5, 2021
Merged
2 tasks
oxarbitrage
oxarbitrage approved these changes Nov 5, 2021
View reviewed changes
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@teor2345 teor2345 merged commit b1303ab into main Nov 5, 2021
@teor2345 teor2345 deleted the compact-size-conversion branch November 5, 2021 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxarbitrage oxarbitrage oxarbitrage approved these changes

Assignees

@teor2345 teor2345

Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow CompactSize to be used for non-length fields (addrv2 and ZSEs)
2 participants
@teor2345 @oxarbitrage