node: updates serialize and deserialize for v2 VAAs by scnale · Pull Request #27 · XLabs/wormhole

scnale · 2025-11-10T14:56:39Z

This keeps the Go struct for VAAs the same but modifies the serialization and deserialization for v2 VAAs. It also errors out on unknown versions.

jonathanMweiss · 2025-11-25T15:09:31Z

I tried running the tss tests (cd to node/pkg/tss then typed go test .) and received the following build error:

../../../sdk/vaa/structs.go:509:72: undefined: i
../../../sdk/vaa/structs.go:740:16: undefined: sig
../../../sdk/vaa/structs.go:745:13: undefined: sig

Copilot

Pull request overview

This PR modifies the VAA serialization and deserialization logic to support v2 VAAs (TSSVaaVersion), which use a Schnorr threshold signature scheme instead of multiple guardian signatures. The changes maintain backward compatibility with v1 VAAs while adding error handling for unsupported versions.

Key changes:

Introduces SchnorrSigLength constant (52 bytes) to define the Schnorr signature size for v2 VAAs
Updates Unmarshal to handle version-specific signature deserialization (multiple signatures for v1, single Schnorr signature for v2)
Updates Marshal to handle version-specific signature serialization with validation checks for v2 VAAs

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-25T15:20:30Z

+	} else if v.Version == 2 {
+		v.Signatures = make([]*Signature, 1)
 		signature := [65]byte{}
-		if n, err := reader.Read(signature[:]); err != nil || n != 65 {
-			return nil, fmt.Errorf("failed to read signature [%d]: %w", i, err)
+		if n, err := reader.Read(signature[:SchnorrSigLength]); err != nil || n != SchnorrSigLength {
+			return nil, fmt.Errorf("failed to read schnorr signature [%d]: %w", i, err)
 		}
-
-		v.Signatures[i] = &Signature{
-			Index:     index,
+		v.Signatures[0] = &Signature{
+			Index:     0,
 			Signature: signature,
 		}


The new v2 VAA deserialization logic lacks test coverage. Consider adding tests that verify:

Correct deserialization of v2 VAAs with 52-byte Schnorr signatures

Error handling for invalid signature lengths

The signature is properly stored in the 65-byte array with zeros in the remaining bytes

Round-trip marshal/unmarshal for v2 VAAs

Copilot · 2025-11-25T15:20:30Z

+	} else if v.Version == 2 {
+		if len(v.Signatures) != 1 {
+			panic(fmt.Errorf("tried to serialize v2 VAA with %d signatures", len(v.Signatures)).Error())
+		}
+
+		zeroSlice := sig.Signature[SchnorrSigLength:]
+		if !bytes.Equal(zeroSlice, make([]byte, len(zeroSlice))) {
+			panic(fmt.Errorf("tried to serialize v2 VAA with invalid signature").Error())
+		}
+
+		buf.Write(sig.Signature[:SchnorrSigLength])


The new v2 VAA serialization logic lacks test coverage. Consider adding tests that verify:

Correct serialization of v2 VAAs with single Schnorr signature

Panic behavior when serializing v2 VAA with multiple signatures

Panic behavior when the signature has non-zero bytes beyond SchnorrSigLength

Round-trip marshal/unmarshal for v2 VAAs

jonathanMweiss · 2026-01-01T11:34:54Z

+		}
+	} else if v.Version == TSSVaaVersion {
+		if len(v.Signatures) != 1 {
+			panic(fmt.Errorf("tried to serialize v2 VAA with %d signatures", len(v.Signatures)).Error())


It is better to return an error rather than panic here (and below, too).

Do you know what kind of errors we should be returning? I never saw code for this so I don't know the best way to do it.

jonathanMweiss · 2026-01-01T11:41:44Z

+
+		sig := v.Signatures[0]
+		zeroSlice := sig.Signature[SchnorrSigLength:]
+		if !bytes.Equal(zeroSlice, make([]byte, len(zeroSlice))) {


I'm not sure I follow this line.
Are you trying to ensure there are no additional bytes in sig.Signature?
If that is the case, wouldn't it be better to check the length of sig.Signature?

The type definition for SignatureData forces it to be of fixed length 65 though, doesn't it? 🤔

I'm a bit wary of changing the type definition. Maybe it's not terrible to do it but I'm a bit unsure it's viable.

jonathanMweiss · 2026-01-01T12:06:32Z

+	// The schnorr signature is constituted by:
+	// - r: 20 bytes
+	// - s: 32 bytes
+	SchnorrSigLength = 52


This is not correct at the moment.

The output of the frost signature is common.SignatureData (which contains both S=32byte and R=33byte).
The processor translates it into a frost.Signature (65 byte), and stores it that way in the VAA (since we didn't export more than one signature verification function).

It is possible to convert the signature to the smart-contract format: frostsig.ToContractSig() returns S, R, and Address(R).

Does the VAA.Marshal() determine how signatures are passed to the smart contract?

Hey, yes, VAAs are the serialized format that gets submitted to smart contracts for verification.

I gather that we may be missing a conversion then, right?

scnale requested review from jonathanMweiss and yossigi November 10, 2025 14:56

jonathanMweiss reviewed Nov 25, 2025

View reviewed changes

Comment thread sdk/vaa/structs.go Outdated

jonathanMweiss reviewed Nov 25, 2025

View reviewed changes

Comment thread sdk/vaa/structs.go Outdated

jonathanMweiss requested a review from Copilot November 25, 2025 15:12

Copilot started reviewing on behalf of jonathanMweiss November 25, 2025 15:13 View session

Copilot finished reviewing on behalf of jonathanMweiss November 25, 2025 15:15

Copilot AI reviewed Nov 25, 2025

View reviewed changes

scnale force-pushed the node/serialize-v2-vaa branch from 334d040 to 5632003 Compare December 17, 2025 13:09

node: updates serialize and deserialize for v2 VAAs

e62b369

scnale force-pushed the node/serialize-v2-vaa branch from 5632003 to e62b369 Compare December 17, 2025 13:12

jonathanMweiss reviewed Jan 1, 2026

View reviewed changes

Conversation

scnale commented Nov 10, 2025

Uh oh!

jonathanMweiss commented Nov 25, 2025

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants