fix: multiple named tuple result from contract

[EnriqueL8]

Fixes EVM queries for:

Functions with multiple return values

function myFunction() public view returns (uint256, uint256) 

Result of JSON will become:

{
  "output": "12345",
  "output1": "12345" // prior to this PR, this value was missing
}

Functions with named return values

function myFunction() public view returns (myvalue uint256) 

{
  "myvalue": "12345" // prior to this PR, this value was missing (empty object returned)
}

Functions with multiple named return values

function myFunction() public view returns (my1 uint256, my2 uint256, my3 uint256) 

{
  "my1": "12345", // all these values were missing (empty object returned)
  "my2": "12345",
  "my3": "12345" 
}

[codecov-commenter]

Codecov Report

Merging #1333 (763db0a) into main (e2f00f0) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 763db0a differs from pull request most recent head 5236926. Consider uploading reports for the commit 5236926 to get more accurate results

@@            Coverage Diff            @@
##              main     #1333   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          314       314           
  Lines        21447     21447           
=========================================
  Hits         21447     21447           

Impacted Files	Coverage Δ
internal/blockchain/ethereum/ethereum.go	100.00% <100.00%> (ø)

[peterbroadhurst]

@EnriqueL8 / @nguyer - this seems to be a fundamental change/enhancement to the API surface area of FireFly core. Think we need to be really clear in what the behavior is today, what this change means, and where this is/should-be documented in the FireFly docs.

Would you mind adding some commentary to that effect?

[peterbroadhurst]

If I understand correctly, the proposal here is:

• If the ABI has a single unnamed parameter, then we return output: {whatever}
• If the ABI has a single named parameter, then we return output: {name1: {whatever}}
• If the ABI has multiple unnamed parameters, then we return output: {output: {whatever1}, output1: {whatever2}}
• If the ABI has multiple named parameters, then we return output: {name1: {whatever1}, name2: {whatever2}}

[EnriqueL8]

Almost there just this one is different:

• If the ABI has multiple unnamed parameters, then we return output: {whatever1}, output1: {whatever2} instead of it being nested.

Discussed this one with @Chengxuan and thought it would be better as above but happy to change

[peterbroadhurst]

ok, so the logic is "if any of the data comes back from the connector contains output as a field, return the whole output unchanged

[peterbroadhurst]

... otherwise, nest whatever came back from the connector under an output parent object

[EnriqueL8]

yes

[EnriqueL8]

That Fabric doc must be out of date then!

It's still the same problem I believe. We are decoding to an object that expects a key "Result"

firefly/internal/blockchain/fabric/fabric.go

Line 102 in e2f00f0

Result interface{} `json:"result"`

so the fabric connector needs to return that. Either the fabric connector is incapsulating everything under that result field or it would hit the same error with multiple values

[EnriqueL8]

I will amend this PR to proxy the direct output for ethereum connector and we can discuss the fabric one as well and decide in a future PR - need to look at the fabric connector code as well

[EnriqueL8]

Fabric construct that object after serialisation https://github.com/hyperledger/firefly-fabconnect/blob/b46951bb6613a2981b89c3ea2e69048441ec41ed/internal/rest/sync/syncdispatcher.go#L200-L203 completely different to ethereum where we add a custom name generator to override the serialisation

[peterbroadhurst]

so the fabric connector needs to return that

Yes, I agree result is where Fabric connector returns it. But that's just an interface{}.

e.g. - in Ethereum today which is broken:

• The connector returns the result at the top level of the JSON, and the result must have exactly one object we care about called output

In Fabric today:

• The connector returns the result in a sub-field of the JSON called result, and can specify anything it likes there which we will return at the top level of the JSON to the end user

[peterbroadhurst]

So, I think if you change Ethereum as follows, it will be very consistent with Fabric:

	var output interface{}
	if err = json.Unmarshal(res.Body(), &output); err != nil {
		return nil, err
	}
	return output, nil // note UNLIKE fabric this is just `output`, not `output.Result` - but either way the top level of what we return to the end user, is whatever the Connector sent us

[EnriqueL8]

This PR changes the contract between Firefly core and the ethereum blockchain connector (EVMConnect and ETHConnect) by relaxing the schema validation when reading the result of a contract query operation.

Before this PR, FF core relied on the result being in the format "output": {} now it can accept any arbitrary map of string to any (interface{}). Yet as the firefly interface nests the result in an "output" we still do it but in ff instead

[peterbroadhurst]

@EnriqueL8 - I believe this should just be interface{} (not a map)

arbitrary map of string to any (interface{}

I agree that's what EVMConnect is configured to return today. But in firefly-signer there are other options (possibly not exposed in config on EVMConnect yet).

For example, you can make it work just like Fabric where it returns an array (not an object) of the values.

I don't see any reason why we'd want to make a change now in Core that precludes EVMConnect being configured by someone to do that in the future.

[EnriqueL8]

Good point! Amending

[peterbroadhurst]

Thanks for working through the approach in the conversation.
Very pleased with where we are landing to simply proxy through for Ethereum what comes from the connector, and I believe it's very consistent with Fabric where the core<->FabConnect contract is just interface{}.

Small request above is to just use interface{} (like Fabric does) rather than map[string]interface{}, so we can support Array returns from Ethereum connectors in the future.

[nguyer]

Resolves #1304

[peterbroadhurst]

👍