[Infra] Modify signaturure verification method

[quantumagi]

@rowandh , besides the points @YakupIpek is making regarding "expected behaviour" there is also the issue of converting the challenge to bytes, which probably can't / shouldn't be done within the contract. As such we should have the option of either passing the challenge/message as bytes or as a string.

Regarding Yakup's points. I thought the same and assumed it was only my opinion, however having this confirmed makes me think most users may feel this way.

[rowandh]

I don't see any unexpected behaviour here. With the new implementation we could also ask the question "When does GetVerifiedSignatures return null?" and the answers are still the same.

We return null when the inputs are invalid, or an exception has been thrown. With TryVerify we return false instead of null to indicate that one of those things has occurred.

It's useful for a contract to know that an operation has failed and the TryVerify pattern is explicit about this, rather than returning a null which the contract will then have to test for.

For this particular method returning null to indicate failure might be fine. For the other parts of the API returning null might be an expected output, and we need another way to indicate that an operation has failed. For consistency with methods like that, either a TryX pattern or a Result<T> pattern seems appropriate. The former is more familiar to developers with a C# background, the latter is more familiar to functional programmers, so I chose the former.

[YakupIpek]

IMO the method should return empty array at worst case otherwise these null values bring null checks everywhere in codebase.

[rowandh]

Empty arrays can also be returned when the parameters are correct and there are no errors. So an empty array could indicate an error, but it could also be a valid result. We need a way to differentiate between the two.

[quantumagi]

So I understand that the main issue is with returning null? Current/typical use-case:

    private void VerifySignatures(byte[] signatures, string authorizationChallenge)
    {
        string[] sigs = this.Serializer.ToArray<string>(signatures);

        var verifieds = ECRecover.GetVerifiedSignatures(sigs, authorizationChallenge, this.Signatories);

        Assert(verifieds?.Length >= this.Quorum, $"Please provide {this.Quorum} valid signatures for '{authorizationChallenge}'.");
    }

[rowandh]

Returning null to indicate an error has occurred, and making the API of this method consistent with the methods of other APIs.

 private void VerifySignatures(byte[] signatures, string authorizationChallenge)
    {
        string[] sigs = this.Serializer.ToArray<string>(signatures);

        Assert(ECRecover.TryGetVerifiedSignatures(sigs, authorizationChallenge, this.Signatories, out var verifieds), "Invalid signatures");

        Assert(verifieds?.Length >= this.Quorum, $"Please provide {this.Quorum} valid signatures for '{authorizationChallenge}'.");
    }

[YakupIpek]

From client perspective empty array will not cause any side effect whether it is empty array becuse of success or failure.

[rowandh]

Sure, maybe not for this particular method. But it will be important somewhere else in the SCL API, and our methods need to be consistent.

[quantumagi]

Does the library support extension methods? E.g.:

public static void VerifySignatures(this SmartContract smartContract, byte[] signatures, string message, Address[] signatories, uint quorum)
{
    string[] sigs = smartContract.Serializer.ToArray<string>(signatures);
    Address[] result = GetVerifiedSignatures(sigs, System.Text.Encoding.ASCII.GetBytes(message), signatories);
    smartContract.Assert(result != null, "Invalid signatures");
    smartContract.Assert(result.Length >= quorum, $"Please provide {quorum} valid signatures for '{message}'.");
}

This way we don't need the VerifySignatures method that's currently in the smart contract?

Seems Assert is protected. Does it need to be?

[rowandh]

The library should not have knowledge of Assert. The library should not do anything to obscure functionality. Burying asserts in extension methods is a side effect and obscures functionality.

[quantumagi]

The library should not have knowledge of Assert. The library should not do anything to obscure functionality. Burying asserts in extension methods is a side effect and obscures functionality.

@rowandh, some things that are common coding practice, such as throwing exceptions from library methods, seem to be frowned upon when it comes to smart contracts. Is there also some weakness or limitation that smart contracts have - thereby making this bad coding practice for smart contracts?

[rowandh]

@quantumagi it's largely to do with the type of programming paradigm that we want people to follow when writing contracts. Because of the inability to easily patch bugs and the potential for funds to be lost, things that behave in unexpected ways can quickly become problematic.

Because of this, we want to guide people to a 'happy path' where it's difficult to make these kinds of errors.

[rowandh]

LGTM