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

Fix tagged message length from int32_t to uint32_t #391

Merged
merged 2 commits into from
Mar 12, 2024

Conversation

Jakio815
Copy link
Collaborator

@Jakio815 Jakio815 commented Mar 11, 2024

This is a fix of #368.

The tagged message sends a message length of int32_t. The length should not be negative, so changing it to uint32_t.

It is both applied on MSG_TYPE MSG_TYPE_TAGGED_MESSAGE and MSG_TYPE_P2P_TAGGED_MESSAGE.

It also makes the maximum size of the tagged message from 2GB to 4GB.

By the way, the typescript target already writes in unsigned int 32 type.

msg.writeUInt32LE(value.length, 5);

@Jakio815 Jakio815 changed the title Draft: Add to make new pr Draft: FIXPR: Fix tagged message length from int32_t to uint32_t Mar 11, 2024
@Jakio815 Jakio815 marked this pull request as ready for review March 11, 2024 19:03
@lhstrh lhstrh requested a review from edwardalee March 11, 2024 21:39
@Jakio815 Jakio815 changed the title Draft: FIXPR: Fix tagged message length from int32_t to uint32_t FIXPR: Fix tagged message length from int32_t to uint32_t Mar 11, 2024
@Jakio815 Jakio815 changed the title FIXPR: Fix tagged message length from int32_t to uint32_t Fix tagged message length from int32_t to uint32_t Mar 11, 2024
@Jakio815 Jakio815 removed the python label Mar 11, 2024
@Jakio815 Jakio815 enabled auto-merge March 12, 2024 15:53
Copy link
Contributor

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

LGTM.

@Jakio815 Jakio815 added this pull request to the merge queue Mar 12, 2024
Merged via the queue into main with commit 14f6d29 Mar 12, 2024
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants