Consistently name multiple returned values

[ernestognw]

Fixes N-05

Took the time to look for every case of multiple return values that weren't named. We can reduce the scope to those of the audit if needed.

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

⚠️ No Changeset found

Latest commit: 9756730

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Amxx]

why ax, ay ? I confused by the a part

• Could be rx ,ad ry for "result"
• Could be vx and vy (because v comes after u)

[ernestognw]

Named them after "affine x" and "affine y". I'm fine with rx and ry

[Amxx]

I'm concerned with the performance impact of this change, because yes, there is an impact.

Lets compare

function double_v1(uint256 X) public returns (uint256) { 
    return 2 * x;
}
function double_v2(uint256 X) public returns (uint256 result) { 
    return 2 * x;
}
function double_v3(uint256 X) public returns (uint256 result) { 
    result = 2 * x;
}

• In the first case, the input is doubled, and put to a temporary (unamed) variable on the stack. This is returned.
• In the second case, the result variable is declared (and reserved) on the stack (that is a push0). Then the input is doubled and put onto a temporary (unamed) variable on the stack, then the temporary variable is moved to the return variable (on the stack) using a swap or something similar. Cleaning the stack might also require a pop. In the end, the value is returned.
• In the third case, the result variable is declared (and reserved) on the stack (that is a push0). Then the input is doubled and put into the reserved space. In the end, the value is returned.

I'm not exactly sure what the compiler optimizes, but option 2 is clearly the worst in terms of stack space usage. Naming the returns creates a stack object in between the arguments of the function and the internal variable of the function. This makes stack too deep happen "faster".

Here is looks like the impact will be negligeable. Probably less then 10/20 gas per function. Possibly 0 with optimisations ?

[ernestognw]

The second case is more expensive, but I'd be fine with the gas increase. Generally, we prefer to follow guidelines over gas savings and here the order of the savings is low. We're not always naming variables, only when there are multiple of them, which doesn't happen often.

Alternative, we can use the 3rd option, but I remember we never liked it for readability. We might agree to do it whenever returned values are named for whatever reason.

[Amxx]

I'd be ok with doing option 2 when that doesn't cause stack too deep errors. In cases where options 2 causes stack to deep error, then I would go for either option 3, or with

function double_v2_inline_commented(uint256 X) public returns (uint256 /*result*/) { 
    return 2 * x;
}

[ernestognw]

In cases where a stack too deep error is caused I'd just go for option 3. We've done that in the past in AccessManager#schedule

[cairoeth]

should the return values of getFull and _getFullAt functions in Time be named as well?