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

handle larger messages #531

Merged
merged 5 commits into from
Mar 15, 2024
Merged

handle larger messages #531

merged 5 commits into from
Mar 15, 2024

Conversation

felicio
Copy link
Collaborator

@felicio felicio commented Mar 14, 2024

No description provided.

Copy link

changeset-bot bot commented Mar 14, 2024

🦋 Changeset detected

Latest commit: c5501e1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@status-im/js Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Mar 14, 2024

@felicio is attempting to deploy a commit to the Status Team on Vercel.

A member of the Team first needs to authorize it.

@felicio felicio self-assigned this Mar 14, 2024
@felicio felicio changed the title draft: handle larger messages handle larger messages Mar 14, 2024
@felicio felicio requested review from a team, jkbktl, prichodko and marcelines March 14, 2024 09:29
@felicio
Copy link
Collaborator Author

felicio commented Mar 14, 2024

note: addressing #529 (comment)

@felicio
Copy link
Collaborator Author

felicio commented Mar 14, 2024

relates: status-im/status-go#4922

Copy link
Collaborator

@marcelines marcelines left a comment

Choose a reason for hiding this comment

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

LGTM. Lack a bit of context but left a comment.

Comment on lines 310 to 324
let decodedMetadata
try {
decodedMetadata = ApplicationMetadataMessage.fromBinary(messageToDecode)
} catch {
return
}

if (!decodedMetadata.payload) {
return
}

if (!decodedMetadata.signature.length) {
return
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Overall, I think the code is a bit verbose and makes it harder to read and maintain. Maybe joining these conditions will help a bit.

  if (!decodedMetadata || !decodedMetadata.payload || !decodedMetadata.signature.length) {
    return;
  }

Also i see you're not handling the error. Wouldn't you use it somehow in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the current context, it's an invalid message that's worth skipping, I believe. I do think it's worth monitoring invalid messages, their size and rate but on anohter level, I'd say.

@felicio felicio merged commit 22e727e into status-im:main Mar 15, 2024
2 of 4 checks passed
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.

3 participants