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

fix: limitation for length of package/realm path #2108

Merged
merged 24 commits into from
May 28, 2024
Merged

fix: limitation for length of package/realm path #2108

merged 24 commits into from
May 28, 2024

Conversation

thinhnx-var
Copy link
Contributor

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

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.

@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label May 15, 2024
@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
limit import path length #2053
Closed
zivkovicmilos
zivkovicmilos previously requested changes May 15, 2024
View reviewed changes
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
petar-dambovaliev
petar-dambovaliev requested changes May 15, 2024
View reviewed changes
Copy link
Contributor

@petar-dambovaliev petar-dambovaliev left a comment •
edited

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.

petar-dambovaliev
petar-dambovaliev approved these changes May 15, 2024
View reviewed changes
tm2/pkg/std/memfile.go Outdated Show resolved Hide resolved
Thinh and others added 2 commits May 16, 2024 09:35
go format
9d00342
@thinhnx-var
refactor testcase
e152af0 
	modified:   tm2/pkg/std/memfile_test.go
@codecov Codecov
Copy link

codecov bot commented May 16, 2024 •
edited

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.

@michelleellen michelleellen mentioned this pull request May 24, 2024
Open
moul
moul reviewed May 26, 2024
View reviewed changes
tm2/pkg/std/memfile.go Outdated Show resolved Hide resolved
moul
moul reviewed May 26, 2024
View reviewed changes
tm2/pkg/std/memfile_test.go Outdated Show resolved Hide resolved
moul
moul reviewed May 26, 2024
View reviewed changes
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.

thehowl
thehowl reviewed May 28, 2024
View reviewed changes
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
@thinhnx-var @moul
@thehowl @omarsy
fix: limitation for length of package/realm path (gnolang#2108)
437a982 
<!-- 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
Reviewers

@moul moul moul left review comments

@linhpn99 linhpn99 linhpn99 left review comments

@thehowl thehowl thehowl approved these changes

@petar-dambovaliev petar-dambovaliev petar-dambovaliev approved these changes

@jaekwon jaekwon Awaiting requested review from jaekwon jaekwon is a code owner

@piux2 piux2 Awaiting requested review from piux2 piux2 is a code owner

@harry-hov harry-hov Awaiting requested review from harry-hov

@zivkovicmilos zivkovicmilos Awaiting requested review from zivkovicmilos zivkovicmilos is a code owner

Assignees

@thinhnx-var thinhnx-var

Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related
Projects
💪 Bounties & Worx
Status: Done
🧙‍♂️gno.land core team
Status: Done
Milestone
🏗4️⃣ test4.gno.land
Development

Successfully merging this pull request may close these issues.

limit import path length
7 participants
@thinhnx-var @moul @thehowl @zivkovicmilos @petar-dambovaliev @linhpn99 @Kouteki