external function invoke() returns always false

[parseb]

warning[5667]: Warning: Unused function parameter. Remove or comment out the variable name to silence this warning.
   --> lib/delegatable-sol/contracts/Delegatable.sol:128:18:
    |
128 |         returns (bool success)
    |                  ^^^^^^^^^^^^

Thesis: invoke function will always return false.
Reasoning: success is declared, returned, but never assigned to.

delegatable-sol/contracts/Delegatable.sol

Lines 125 to 141 in d12016f

	function invoke(SignedInvocation[] calldata signedInvocations)
	external
	override
	returns (bool success)
	{
	for (uint256 i = 0; i < signedInvocations.length; i++) {
	SignedInvocation calldata signedInvocation = signedInvocations[i];
	address invocationSigner = verifyInvocationSignature(
	signedInvocation
	);
	_enforceReplayProtection(
	invocationSigner,
	signedInvocations[i].invocations.replayProtection
	);
	_invoke(signedInvocation.invocations.batch, invocationSigner);
	}
	}

Solution, maybe:

success = i == 0 ?
 _invoke(signedInvocation.invocations.batch, invocationSigner) : success && _invoke(signedInvocation.invocations.batch, invocationSigner);

[danfinlay]

Invoke also doesn't really need to return anything, so we could just remove it from the method definition?

[parseb]

Not sure.


Long temporary take (to consider):
execution success trickling is important for secure composability, or at least nice to have
( I do not know why ERC20 returns true on transfer but ERC721 does not. I think it makes life unnecessarily hard.)

Lastly, thinking about chained delegations, where the invocation would be chained (?) and dependent on one-another -> the successful invocation of all items in a batch would be required. But not sure if I have enough of an understanding of how that would happen atm. I'll look at it later.

[danfinlay]

Today all batches of invocations are atomic and require success, so you can assume they will all succeed as a group. I considered early on making this a parameter, and letting users opt out of atomicity, but I think I didn't bother for simplicity. You can always submit more batches if you want non-atomicity.

Since atomicity is ensured, we might not need the return value?

[parseb]

Since atomicity is ensured, we might not need the return value?

Yes. To the best of my knowledge.

Batch building might require use case specific validation or filtering.
I'm thinking any default or indiscriminate batching is likely to result in griefing attacks.

[McOso]

@parseb This actually isn't true. The external invoke function returns true or false based on the return of the internal _invoke function. It is a bit hard to see but since the return variable is declared, whatever gets returned by the function is set to the return variable. Therefore, the result of the _invoke function of DelegatableCore is set to success.

I find this warning annoying, so I made a quick change to explicitly set the success variable. I think it is worth the additional 3 gas. Will make PR after checking with code owners first.

@danfinlay I think we keep the bool return of external invoke function to allow for the flexibility of a future use case.

[parseb]

@McOso Not sure what you are saying.
whatever gets returned by the function is set to the return variable
success is always false in the above example. As such, the function always returns false.
Solidity doesn't have implicit return and the return of _invoke is never assigned to success.
I might be wrong about the above or you are confounding the lower level .call() return with the declared return.

--
The external invoke function returns true or false based on the return of the internal _invoke function. - This statement is false. Your solution reflects this.