From 41ff3747a554a988777a21046f0bad4345675fc7 Mon Sep 17 00:00:00 2001 From: jon roethke Date: Thu, 21 Mar 2024 03:57:45 -0700 Subject: [PATCH] feat(pkg/std): ensure files are sorted in a `MemPackage` (#1618) This PR ensures in `MemPackage.Validate` that the files that it is being passed are correctly sorted. This is called in many places, among which the VM keeper. **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](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
--------- Co-authored-by: Morgan Bazalgette --- gno.land/pkg/sdk/vm/keeper.go | 1 + tm2/pkg/std/memfile.go | 38 +++++++++++++++++-------- tm2/pkg/std/memfile_test.go | 53 +++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 11 deletions(-) create mode 100644 tm2/pkg/std/memfile_test.go diff --git a/gno.land/pkg/sdk/vm/keeper.go b/gno.land/pkg/sdk/vm/keeper.go index 54f424ee058..67710be620c 100644 --- a/gno.land/pkg/sdk/vm/keeper.go +++ b/gno.land/pkg/sdk/vm/keeper.go @@ -174,6 +174,7 @@ func (vm *VMKeeper) AddPackage(ctx sdk.Context, msg MsgAddPackage) error { if err != nil { return err } + // Parse and run the files, construct *PV. msgCtx := stdlibs.ExecContext{ ChainID: ctx.ChainID(), diff --git a/tm2/pkg/std/memfile.go b/tm2/pkg/std/memfile.go index 599e9a59cc5..5935428ac28 100644 --- a/tm2/pkg/std/memfile.go +++ b/tm2/pkg/std/memfile.go @@ -3,9 +3,8 @@ package std import ( "fmt" "regexp" + "sort" "strings" - - "github.com/gnolang/gno/tm2/pkg/errors" ) type MemFile struct { @@ -50,22 +49,39 @@ var ( // file names must contain dots. // NOTE: this is to prevent conflicts with nested paths. func (mempkg *MemPackage) Validate() error { + // add assertion that MemPkg contains at least 1 file + if len(mempkg.Files) <= 0 { + return fmt.Errorf("no files found within package %q", mempkg.Name) + } + if !rePkgName.MatchString(mempkg.Name) { - return errors.New(fmt.Sprintf("invalid package name %q, failed to match %q", mempkg.Name, rePkgName)) + return fmt.Errorf("invalid package name %q, failed to match %q", mempkg.Name, rePkgName) } if !rePkgOrRlmPath.MatchString(mempkg.Path) { - return errors.New(fmt.Sprintf("invalid package/realm path %q, failed to match %q", mempkg.Path, rePkgOrRlmPath)) + return fmt.Errorf("invalid package/realm path %q, failed to match %q", mempkg.Path, rePkgOrRlmPath) } - fnames := map[string]struct{}{} - for _, memfile := range mempkg.Files { - if !reFileName.MatchString(memfile.Name) { - return errors.New(fmt.Sprintf("invalid file name %q, failed to match %q", memfile.Name, reFileName)) + // enforce sorting files based on Go conventions for predictability + sorted := sort.SliceIsSorted( + mempkg.Files, + func(i, j int) bool { + return mempkg.Files[i].Name < mempkg.Files[j].Name + }, + ) + if !sorted { + return fmt.Errorf("mempackage %q has unsorted files", mempkg.Path) + } + + var prev string + for i, file := range mempkg.Files { + if !reFileName.MatchString(file.Name) { + return fmt.Errorf("invalid file name %q, failed to match %q", file.Name, reFileName) } - if _, exists := fnames[memfile.Name]; exists { - return errors.New(fmt.Sprintf("duplicate file name %q", memfile.Name)) + if i > 0 && prev == file.Name { + return fmt.Errorf("duplicate file name %q", file.Name) } - fnames[memfile.Name] = struct{}{} + prev = file.Name } + return nil } diff --git a/tm2/pkg/std/memfile_test.go b/tm2/pkg/std/memfile_test.go new file mode 100644 index 00000000000..44031cd1b8b --- /dev/null +++ b/tm2/pkg/std/memfile_test.go @@ -0,0 +1,53 @@ +package std + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestMemPackage_Validate(t *testing.T) { + tt := []struct { + name string + mpkg *MemPackage + errContains string + }{ + { + "Correct", + &MemPackage{ + Name: "hey", + Path: "gno.land/r/demo/hey", + Files: []*MemFile{{Name: "a.gno"}}, + }, + "", + }, + { + "Unsorted", + &MemPackage{ + Name: "hey", + Path: "gno.land/r/demo/hey", + Files: []*MemFile{{Name: "b.gno"}, {Name: "a.gno"}}, + }, + `mempackage "gno.land/r/demo/hey" has unsorted files`, + }, + { + "Duplicate", + &MemPackage{ + Name: "hey", + Path: "gno.land/r/demo/hey", + Files: []*MemFile{{Name: "a.gno"}, {Name: "a.gno"}}, + }, + `duplicate file name "a.gno"`, + }, + } + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + err := tc.mpkg.Validate() + if tc.errContains == "" { + assert.NoError(t, err) + } else { + assert.ErrorContains(t, err, tc.errContains) + } + }) + } +}