From beae4f70e2adf646c16647b7b75631a68d4f88bf Mon Sep 17 00:00:00 2001 From: grepsuzette Date: Tue, 11 Jun 2024 12:21:44 +0800 Subject: [PATCH 1/5] chore(docs): document frequent functions in tm2/pkg/amino --- tm2/pkg/amino/README.md | 4 ++-- tm2/pkg/amino/amino.go | 10 +++++++++- tm2/pkg/amino/pkg/pkg.go | 15 +++++++++++++++ 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/tm2/pkg/amino/README.md b/tm2/pkg/amino/README.md index 1a69cc7426c..82b89f3455f 100644 --- a/tm2/pkg/amino/README.md +++ b/tm2/pkg/amino/README.md @@ -85,8 +85,8 @@ var Package = amino.RegisterPackage( ) ``` -You can still override global registrations with local \*amino.Codec state. -This is used by genproto.P3Context, which may help development while writing +You can still override global registrations with local `*amino.Codec` state. +This is used by `genproto.P3Context`, which may help development while writing migration scripts. Feedback welcome in the issues section. ## Unsupported types diff --git a/tm2/pkg/amino/amino.go b/tm2/pkg/amino/amino.go index 1ca3427b85c..b1552904167 100644 --- a/tm2/pkg/amino/amino.go +++ b/tm2/pkg/amino/amino.go @@ -873,17 +873,25 @@ func (cdc *Codec) MarshalJSONIndent(o interface{}, prefix, indent string) ([]byt // ---------------------------------------- // Other +// Given amino package `pi`, register it with the global codec. // NOTE: do not modify the result. func RegisterPackage(pi *pkg.Package) *Package { gcdc.RegisterPackage(pi) return pi } +// Create an unregistered amino package with args: +// - (gopkg string) The Go package path, e.g. "github.com/gnolang/gno/tm2/pkg/std" +// - (p3pkg string) The (shorter) Proto3 package path (no slashes), e.g. "std" +// - (dirname string) Package directory this is called from. Typical is to use `amino.GetCallersDirname()` func NewPackage(gopkg string, p3pkg string, dirname string) *Package { return pkg.NewPackage(gopkg, p3pkg, dirname) } -// NOTE: duplicated in pkg/pkg.go +// Get caller's package directory. +// Implementation uses `filepath.Dir(runtime.Caller(1))`. +// NOTE: duplicated in pkg/pkg.go; given what it does and how, +// both are probably needed. func GetCallersDirname() string { dirname := "" // derive from caller. _, filename, _, ok := runtime.Caller(1) diff --git a/tm2/pkg/amino/pkg/pkg.go b/tm2/pkg/amino/pkg/pkg.go index b6ffab9748e..94e1a4c08be 100644 --- a/tm2/pkg/amino/pkg/pkg.go +++ b/tm2/pkg/amino/pkg/pkg.go @@ -118,11 +118,26 @@ func (pkg *Package) WithGoPkgName(name string) *Package { return pkg } +// Package dependencies need to be declared (for now). +// If a package has no dependency, it is conventional to +// use `.WithDependencies()` with no arguments. func (pkg *Package) WithDependencies(deps ...*Package) *Package { pkg.Dependencies = append(pkg.Dependencies, deps...) return pkg } +// WithType() specifies which types are encoded and decoded by the package. +// You must provide a list of instanciated objects in the arguments. +// Each type declaration may be optionally followed by a string which is then +// used as its name. +// +// E.g. .WithTypes( +// +// StructA{}, +// &StructB{}, // If pointer receivers are preferred when decoding to interfaces. +// NoInputsError{}, "NoInputsError", // Named +// +// ) func (pkg *Package) WithTypes(objs ...interface{}) *Package { var lastType *Type = nil for _, obj := range objs { From 67cc77644d8992983ad1f6c47b53baf889a85f37 Mon Sep 17 00:00:00 2001 From: grepsuzette Date: Tue, 11 Jun 2024 18:58:48 +0800 Subject: [PATCH 2/5] chore(amino): add some tests Trying to address an ancien line in amino: XXX Test registering duplicate names or concrete types not in a package. Unfortunately one of them fails to panic: the problem is names aren't tested in `WithTypes()` The failing test is commented out for now --- tm2/pkg/amino/codec_test.go | 66 +++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/tm2/pkg/amino/codec_test.go b/tm2/pkg/amino/codec_test.go index 9368d4ef40e..f0dddc6c361 100644 --- a/tm2/pkg/amino/codec_test.go +++ b/tm2/pkg/amino/codec_test.go @@ -3,6 +3,7 @@ package amino_test import ( "bytes" "encoding/binary" + "reflect" "strings" "testing" "time" @@ -236,4 +237,69 @@ func TestCodecSeal(t *testing.T) { assert.Panics(t, func() { cdc.RegisterPackage(tests.Package) }) } +func TestDupTypesMustPanic(t *testing.T) { + // duplicate types must panic + t.Parallel() + + pkg := amino.NewPackage( + reflect.TypeOf(SimpleStruct{}).PkgPath(), + "amino_test", + amino.GetCallersDirname(), + ) + assert.PanicsWithError( + t, + "type amino_test.SimpleStruct already registered with package", + func() { + pkg.WithTypes( + SimpleStruct{}, + SimpleStruct{}, + ) + }) +} + +func TestTypesOutsidePackageMustPanic(t *testing.T) { + // adding concrete types from within a different package must panic + // (use dependency instead) + t.Parallel() + + makepkg := func() *amino.Package { + return amino.NewPackage( + reflect.TypeOf(tests.EmptyStruct{}).PkgPath(), + "amino_test", + amino.GetCallersDirname(), + ) + } + + makepkg().WithTypes(tests.PrimitivesStruct{}) // from same package ✓ + + assert.Panics(t, func() { + makepkg().WithTypes( + SimpleStruct{}, // from another package ✗ + ) + }) +} + +func TestDupNamesMustPanic(t *testing.T) { + // adding types with the same names must panic + t.Parallel() + + makepkg := func() *amino.Package { + return amino.NewPackage( + reflect.TypeOf(tests.EmptyStruct{}).PkgPath(), + "amino_test", + amino.GetCallersDirname(), + ) + } + // BUG currently following new test does NOT panic. + // Commenting out so it doesn't break for now. + // + // assert.Panics(t, func() { + // makepkg().WithTypes( + // tests.EmptyStruct{}, "A", + // tests.PrimitivesStruct{}, "B", + // tests.ShortArraysStruct{}, "A", // Same name! + // ) + // }) +} + // XXX Test registering duplicate names or concrete types not in a package. From 337226a3ed35087d07b43ed6eadaeb9b6a459f82 Mon Sep 17 00:00:00 2001 From: grepsuzette Date: Tue, 11 Jun 2024 19:42:38 +0800 Subject: [PATCH 3/5] amend last commit --- tm2/pkg/amino/codec_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tm2/pkg/amino/codec_test.go b/tm2/pkg/amino/codec_test.go index f0dddc6c361..abf7d2289ea 100644 --- a/tm2/pkg/amino/codec_test.go +++ b/tm2/pkg/amino/codec_test.go @@ -290,8 +290,13 @@ func TestDupNamesMustPanic(t *testing.T) { amino.GetCallersDirname(), ) } - // BUG currently following new test does NOT panic. - // Commenting out so it doesn't break for now. + makepkg().WithTypes( + tests.EmptyStruct{}, "A", + tests.PrimitivesStruct{}, "B", + tests.ShortArraysStruct{}, "C", + ) + // BUG? currently following new test does NOT panic. + // Commenting out so it doesn't break. // // assert.Panics(t, func() { // makepkg().WithTypes( From b1f057daf3dc756c30b4731fdff3224a9e111adb Mon Sep 17 00:00:00 2001 From: Morgan Bazalgette Date: Tue, 22 Oct 2024 06:47:55 -0500 Subject: [PATCH 4/5] fix bug --- tm2/pkg/amino/codec_test.go | 19 +++++++------------ tm2/pkg/amino/pkg/pkg.go | 11 +++++++++++ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/tm2/pkg/amino/codec_test.go b/tm2/pkg/amino/codec_test.go index abf7d2289ea..89ced32b064 100644 --- a/tm2/pkg/amino/codec_test.go +++ b/tm2/pkg/amino/codec_test.go @@ -295,16 +295,11 @@ func TestDupNamesMustPanic(t *testing.T) { tests.PrimitivesStruct{}, "B", tests.ShortArraysStruct{}, "C", ) - // BUG? currently following new test does NOT panic. - // Commenting out so it doesn't break. - // - // assert.Panics(t, func() { - // makepkg().WithTypes( - // tests.EmptyStruct{}, "A", - // tests.PrimitivesStruct{}, "B", - // tests.ShortArraysStruct{}, "A", // Same name! - // ) - // }) + assert.Panics(t, func() { + makepkg().WithTypes( + tests.EmptyStruct{}, "A", + tests.PrimitivesStruct{}, "B", + tests.ShortArraysStruct{}, "A", // Same name! + ) + }) } - -// XXX Test registering duplicate names or concrete types not in a package. diff --git a/tm2/pkg/amino/pkg/pkg.go b/tm2/pkg/amino/pkg/pkg.go index 94e1a4c08be..93cd7c1036d 100644 --- a/tm2/pkg/amino/pkg/pkg.go +++ b/tm2/pkg/amino/pkg/pkg.go @@ -198,6 +198,17 @@ func (pkg *Package) WithTypes(objs ...interface{}) *Package { } pkg.Types = append(pkg.Types, lastType) } + seen := make(map[string]struct{}) + for _, tp := range pkg.Types { + // tp.Name is "" for cases like nativePkg, containing go native types. + if tp.Name == "" { + continue + } + if _, ok := seen[tp.Name]; ok { + panic("duplicate type name " + tp.Name) + } + seen[tp.Name] = struct{}{} + } return pkg } From e59f434ef5ec0ee85767aeeabc0608e8f93d21a3 Mon Sep 17 00:00:00 2001 From: Morgan Bazalgette Date: Tue, 22 Oct 2024 06:50:13 -0500 Subject: [PATCH 5/5] fixup godoc --- tm2/pkg/amino/pkg/pkg.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tm2/pkg/amino/pkg/pkg.go b/tm2/pkg/amino/pkg/pkg.go index 93cd7c1036d..7a31256f184 100644 --- a/tm2/pkg/amino/pkg/pkg.go +++ b/tm2/pkg/amino/pkg/pkg.go @@ -126,18 +126,16 @@ func (pkg *Package) WithDependencies(deps ...*Package) *Package { return pkg } -// WithType() specifies which types are encoded and decoded by the package. -// You must provide a list of instanciated objects in the arguments. +// WithType specifies which types are encoded and decoded by the package. +// You must provide a list of instantiated objects in the arguments. // Each type declaration may be optionally followed by a string which is then // used as its name. // -// E.g. .WithTypes( -// -// StructA{}, -// &StructB{}, // If pointer receivers are preferred when decoding to interfaces. -// NoInputsError{}, "NoInputsError", // Named -// -// ) +// pkg.WithTypes( +// StructA{}, +// &StructB{}, // If pointer receivers are preferred when decoding to interfaces. +// NoInputsError{}, "NoInputsError", // Named +// ) func (pkg *Package) WithTypes(objs ...interface{}) *Package { var lastType *Type = nil for _, obj := range objs {