Is the conversion of transaction signer to stackitem implemented as intended?

[AnnaShaleva]

Problem description

#2685 introduces new GetTransactionSigners method to native Ledger. Take a look at the way how each signer is converted to stackitem: the first array item is the serialized binary representation of the Signer itself:

neo/src/neo/Network/P2P/Payloads/Signer.cs

Lines 187 to 191 in ed68274

	VM.Types.StackItem IInteroperable.ToStackItem(ReferenceCounter referenceCounter)
	{
	return new VM.Types.Array(referenceCounter, new VM.Types.StackItem[]
	{
	this.ToArray(),

The binary representation is followed by a set of Signer's fields serialized to stackitem separately:

neo/src/neo/Network/P2P/Payloads/Signer.cs

Lines 192 to 197 in ed68274

	Account.ToArray(),
	(byte)Scopes,
	new VM.Types.Array(AllowedContracts.Select(u => new VM.Types.ByteString(u.ToArray()))),
	new VM.Types.Array(AllowedGroups.Select(u => new VM.Types.ByteString(u.ToArray()))),
	new VM.Types.Array(Rules.Select(u => u.ToStackItem(referenceCounter)))
	});

So the first stackitem (binary Signer's representation) looks weird and redundant a bit. We have all other Signer's data available at the same stackitem, so why do we need its serialized binary representation?

The question

Do we need the Signer's binary representation to be included in its stackitem representation? If so, could you, please, explain, why?

[shargon]

Agree, the first version only wanted the binary, now it's not required

[erikzhang]

I think we can keep the binary data for convenience.

[roman-khimov]

To me it's just duplicating for no good reason, interpreting these bytes is time-consuming and error-prone activity that is easily avoided by using proper stack item representation (which we already have anyway).

[erikzhang]

In some cases, we can use a single EQUAL instruction to determine whether it is the signer we want without traversing every field of it.

[erikzhang]

In some cases, we can use a single EQUAL instruction to determine whether it is the signer we want without traversing every field of it.

What do you think? We need to decide before 3.3.0 is released.

[roman-khimov]

Doesn't seem to be a useful scenario to me.
🚀: drop binary (merge #2708)
🎉: keep binary (close #2708)