Skip to content

support proto serialization #509

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

Open
krehermann wants to merge 2 commits into
base: main
Choose a base branch
from
Open

support proto serialization #509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
796c406
support proto serialization
krehermann Oct 10, 2025
f98c206
Update operations/validation.go
krehermann Oct 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions operations/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,16 @@ import (
"encoding/json"
"reflect"

"github.com/gogo/protobuf/proto"

"github.com/smartcontractkit/chainlink-common/pkg/logger"
)

// IsSerializable returns true if the value can be marshaled and unmarshaled without losing information, false otherwise.
// For idempotency and reporting purposes, we need to ensure that the value can be marshaled and unmarshaled
// without losing information.
// If the value implements json.Marshaler and json.Unmarshaler, it is assumed to be serializable.
// If the value is a protobuf message, it is also considered serializable.
func IsSerializable(lggr logger.Logger, v any) bool {
if !isValueSerializable(lggr, reflect.ValueOf(v)) {
return false
Expand All @@ -25,11 +28,19 @@ func IsSerializable(lggr logger.Logger, v any) bool {
return true
}

func isProtoMessage(v reflect.Value) bool {
protoMsgType := reflect.TypeOf((*proto.Message)(nil)).Elem()
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function doesn't handle nil pointer values correctly. If v represents a nil pointer to a proto message, v.Type() will return the pointer type but the check may fail. Consider adding a nil check and handling the case where v.Kind() == reflect.Ptr && v.IsNil().

Suggested change
protoMsgType := reflect.TypeOf((*proto.Message)(nil)).Elem()
protoMsgType := reflect.TypeOf((*proto.Message)(nil)).Elem()
// Handle nil pointer: check the type the pointer points to
if v.Kind() == reflect.Ptr && v.IsNil() {
elemType := v.Type().Elem()
return elemType.Implements(protoMsgType) || reflect.PointerTo(elemType).Implements(protoMsgType)
}

Copilot uses AI. Check for mistakes.
return v.Type().Implements(protoMsgType) || reflect.PointerTo(v.Type()).Implements(protoMsgType)
Comment on lines +32 to +33
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reflect.TypeOf((*proto.Message)(nil)).Elem() call is executed on every invocation. Consider extracting this to a package-level variable to avoid repeated reflection operations.

Copilot uses AI. Check for mistakes.
}

func isValueSerializable(lggr logger.Logger, v reflect.Value) bool {
// Handle nil values
if !v.IsValid() {
return true
}
if isProtoMessage(v) {
return true
}
Comment on lines +41 to +43
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proto message check should be placed after the nil value check but before dereferencing pointers to maintain consistent logic flow. Currently, it may incorrectly handle pointer-to-proto cases that are checked later in the function.

Copilot uses AI. Check for mistakes.

// Check if type implements json.Marshaler and json.Unmarshaler
fieldTypeRef := v.Type()
Expand Down
24 changes: 23 additions & 1 deletion operations/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@ import (

"github.com/Masterminds/semver/v3"
chainsel "github.com/smartcontractkit/chain-selectors"
"github.com/smartcontractkit/chainlink-common/pkg/logger"
mcmslib "github.com/smartcontractkit/mcms"
"github.com/smartcontractkit/mcms/types"
"github.com/stretchr/testify/require"

"github.com/smartcontractkit/chainlink-common/pkg/logger"

"github.com/smartcontractkit/chainlink-deployments-framework/datastore"
pb "github.com/smartcontractkit/chainlink-protos/op-catalog/v1/datastore"
)

func Test_IsSerializable(t *testing.T) {
Expand Down Expand Up @@ -151,6 +153,26 @@ func Test_IsSerializable(t *testing.T) {
v: datastore.AddressRef{},
want: true,
},
{
name: "should serialize proto message pointer",
// this is arbitrary proto message just to test the code path
v: &pb.AddressReferenceEditResponse{
Copy link
Collaborator

@graham-chainlink graham-chainlink Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm i dont think this is enough, even though this serialized without error, it doesnt mean we dont have data lost.

My understanding that for protos , we should at least be using "google.golang.org/protobuf/encoding/protojson" as using encoding/json will cause data lost as it doesnt know how to handle proto message.

Record: &pb.AddressReference{
Domain: "anything",
},
},
want: true,
},
{
name: "should serialize proto message value",
// this is arbitrary proto message just to test the code path
v: pb.AddressReferenceEditResponse{
Record: &pb.AddressReference{
Domain: "anything",
},
},
want: true,
},
}

for _, tt := range tests {
Expand Down
Loading