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

Increase Memo, lower Receiver max char lengths #1255

Closed
wants to merge 2 commits into from

Conversation

PoolPirate
Copy link

@PoolPirate PoolPirate commented Aug 6, 2024

Context and purpose of the change

The character limits for IBC packets are necessary, however the current size limit of 2000 chars is too small.
There are valid and reasonable transactionsusing ibc:hooks that can exceed that limit.

On the other hand the receiver field limit is set too high as there is no real reason for addresses to even reach the 2000 char limit.

Brief Changelog

  • Increase max memo length 2000 -> 4096
  • Decrease max receiver length 2000 -> 1024

Author's Checklist

I have...

  • Run and PASSED locally all GAIA integration tests
  • If the change is contentful, I either:
    • Added a new unit test OR
    • Added test cases to existing unit tests
  • OR this change is a trivial rework / code cleanup without any test coverage

(Don't have a local test setup for these, would be nice if they could be ran by somebody else)

Reviewers Checklist

I have...

  • reviewed state machine logic
  • reviewed API design and naming
  • manually tested (if applicable)
  • confirmed the author wrote unit tests for new logic
  • reviewed documentation exists and is accurate

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md?
  • This pull request updates existing proto field values (and require a backend and frontend migration)?
  • Does this pull request change existing proto field names (and require a frontend migration)?
    How is the feature or change documented?
    • not applicable
    • jira ticket XXX
    • specification (x/<module>/spec/)
    • README.md
    • not documented

Copy link
Collaborator

@sampocs sampocs 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 flagging this!

The memo change looks good, but we might have some host zones who are still on older versions of IBC without a memo field (in which case we need to use the receiver as the memo)

I’ll look up what IBC versions our host zones are on and then circle back!

@sampocs
Copy link
Collaborator

sampocs commented Aug 27, 2024

@PoolPirate how did you arrive at the value of 4096?

Also, do you see any reason to support a receiver address over 100 characters? (assuming we use only the memo for autopilot moving forward)

@PoolPirate
Copy link
Author

The longest call chains I could reasonably find so far with skip go were in the 3000 character range. No particular reason for the 96 other than it being 2^12 tho.

Don't see any reason not to decrease the receiver further to 100 chars if more is no longer needed by autopilot.

@sampocs
Copy link
Collaborator

sampocs commented Aug 28, 2024

Closing in favor of #1273

thanks again @PoolPirate!

@sampocs sampocs closed this Aug 28, 2024
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.

2 participants