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: limitation for length of package/realm path #2108

Merged
merged 24 commits into from
May 28, 2024

Conversation

thinhnx-var
Copy link
Contributor

@thinhnx-var thinhnx-var commented May 15, 2024

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@thinhnx-var thinhnx-var changed the title limit package path length to 256 fix: limitation for length of package/realm path May 15, 2024
@thinhnx-var
Copy link
Contributor Author

relate to #2053

@harry-hov harry-hov self-requested a review May 15, 2024 07:40
@Kouteki Kouteki added this to the 🏗4️⃣ test4.gno.land milestone May 15, 2024
@Kouteki Kouteki linked an issue May 15, 2024 that may be closed by this pull request
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Thank you for adding this guard 🙏

There are some things that need to be addressed before moving forward, but overall the PR looks good 💯

tm2/pkg/std/memfile.go Outdated Show resolved Hide resolved
tm2/pkg/std/memfile.go Outdated Show resolved Hide resolved
tm2/pkg/std/memfile.go Outdated Show resolved Hide resolved
tm2/pkg/std/memfile_test.go Outdated Show resolved Hide resolved
@petar-dambovaliev petar-dambovaliev self-requested a review May 15, 2024 14:06
Copy link
Contributor

@petar-dambovaliev petar-dambovaliev left a comment

Choose a reason for hiding this comment

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

Here, we are trying to kill a fly with a thermal nuclear weapon (a regex).
There are 2 ways this can be implemented reasonably.

  1. calling the builtin function len and the limit being imposed on the number of bytes
  2. calling utf8.RuneCountInString and the limit being imposed on the number of characters

I personally think you should use len and limit the path to 256 bytes because it still does the job but it is much faster than the alternative.

@thinhnx-var
Copy link
Contributor Author

Here, we are trying to kill a fly with a thermal nuclear weapon (a regex). There are 2 ways this can be implemented reasonably.

  1. calling the builtin function len and the limit being imposed on the number of bytes
  2. calling utf8.RuneCountInString and the limit being imposed on the number of characters

I personally think you should use len and limit the path to 256 bytes because it still does the job but it is much faster than the alternative.

I think bytes limitation is good for this. I will update it.

@thinhnx-var
Copy link
Contributor Author

@zivkovicmilos @petar-dambovaliev
Please take a look on new updated commit. Thank you sir.

tm2/pkg/std/memfile.go Outdated Show resolved Hide resolved
Thinh and others added 2 commits May 16, 2024 09:35
	modified:   tm2/pkg/std/memfile_test.go
Copy link

codecov bot commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.93%. Comparing base (5924df8) to head (5350429).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2108   +/-   ##
=======================================
  Coverage   49.93%   49.93%           
=======================================
  Files         576      576           
  Lines       77825    77827    +2     
=======================================
+ Hits        38862    38863    +1     
  Misses      35837    35837           
- Partials     3126     3127    +1     
Flag Coverage Δ
tm2 54.47% <100.00%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tm2/pkg/std/memfile.go Outdated Show resolved Hide resolved
tm2/pkg/std/memfile_test.go Outdated Show resolved Hide resolved
tm2/pkg/std/memfile_test.go Outdated Show resolved Hide resolved
@thinhnx-var
Copy link
Contributor Author

@moul
I have just done updated the test file as your review.

tm2/pkg/std/memfile.go Outdated Show resolved Hide resolved
tm2/pkg/std/memfile_test.go Outdated Show resolved Hide resolved
@thehowl thehowl merged commit f43cbfa into gnolang:master May 28, 2024
6 checks passed
omarsy pushed a commit to TERITORI/gno that referenced this pull request Jun 3, 2024
<!-- please provide a detailed description of the changes made in this
pull request. -->

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Co-authored-by: Thinh <thinhnx@192.168.1.9>
Co-authored-by: Thinh <thinhnx@thinhnx.local>
Co-authored-by: Thinh <thinhnx@192.168.1.6>
Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>
Co-authored-by: Morgan Bazalgette <morgan@morganbaz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

limit import path length
7 participants