gno
gno copied to clipboard
feat(vm): sort package files `AddPackage`
This PR introduces sorting of files during vm.AddPackage ensuring that the vm processes files in accordance with Go's lexical conventions.
Context: It was discovered that during the add package process, gno files get processed in an unpredictable order.
This relates to the below related issue (1482) regarding multiple init statements being allowed since these init statements would potentially be processed in unknown order.
Related: https://github.com/gnolang/gno/issues/1482
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: xxxmessage 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, if any. More info here.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 47.49%. Comparing base (
7596f42) to head (9e8b415).
Additional details and impacted files
@@ Coverage Diff @@
## master #1618 +/- ##
==========================================
+ Coverage 44.79% 47.49% +2.70%
==========================================
Files 459 388 -71
Lines 67642 61304 -6338
==========================================
- Hits 30300 29117 -1183
+ Misses 34808 29749 -5059
+ Partials 2534 2438 -96
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I think perhaps the only change necessary is to add something like this:
sort.Slice( msg.Package.Files, func(i, j int) bool { return msg.Package.Files[i].Name < msg.Package.Files[j].Name }, )That should sort the package files in place as long as the original order doesn't need to be preserved, but I don't think it does.
I was thinking copy and replace versus altering in place for safety but this is a significantly more elegant solution.
I think to address your comment about ordering and doing a small performance improvement, too:
- Move the sorting into
std.MemPackage.Validate(this will eventually be moved tosdk/vm, per this comment discussed with Jae)- After sorting, change the current check for duplicates in
Validate()(the one using a map) to a simpler one:// XXX: add assertion that MemPkg contains at least 1 file prev := mempkg.Files[0].Name for _, file := range mempkg.Files[1:] { if prev == file.Name { // return error } prev = file.Name }(The following works because the slice is sorted)
@thehowl Change to your suggestion or merge as is based on previous approvals?
@waymobetta change it to mine, we get to kill two birds with one stone