gno icon indicating copy to clipboard operation
gno copied to clipboard

feat(vm): sort package files `AddPackage`

Open waymobetta opened this issue 1 year ago • 4 comments
trafficstars

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: 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, if any. More info here.

waymobetta avatar Feb 02 '24 01:02 waymobetta

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.

codecov[bot] avatar Feb 02 '24 01:02 codecov[bot]

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.

waymobetta avatar Feb 02 '24 04:02 waymobetta

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 to sdk/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 avatar Feb 20 '24 15:02 waymobetta

@waymobetta change it to mine, we get to kill two birds with one stone

thehowl avatar Feb 20 '24 22:02 thehowl