[P1-N09] Only return named variables #1229
Conversation
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
chrismaree
changed the title
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
| @@ -15,17 +15,17 @@ contract("ResultComputation", function(accounts) { | |||
| await resultComputation.wrapAddVote(priceOne, web3.utils.toWei("5")); | |||
| // Frequency table: priceOne->5. Cutoff: 2.5. | |||
| let resolved = await resultComputation.wrapGetResolvedPrice(minVoteThreshold); | |||
| assert.isTrue(resolved.isResolved); | |||
| assert.equal(resolved.price, priceOne); | |||
| assert.isTrue(resolved[0]); | |||
There was a problem hiding this comment.
@mrice32, @nicholaspai and @ptare if we make this change we can't directly refer to variables if the function called returns more than one variable. I'd like to get your input before making this change. Options are:
- revert to returning named variables. This would mean that we are inconsistent. might be acceptable iff the function returns two separate variables. rule is then that if it has two we use named variables.
- accept the small hit in code readability and ensure to always add comments for code readability.
There was a problem hiding this comment.
After talking to @mrice32 and @nicholaspai about this we converged on two different opinions between readability and consistency. @mrice32's preference is readability and @nicholaspai's preference is consistency.
While it's inconsistent to have named return types in some functions and not others, I feel the benefit in integrating with the contracts with simple named values outways the inconsistency cost. In all other cases having a named return variable is the desired pattern.
Going forward as a styling rule we will now always used named return variables in all cases EXCEPT when returning multiple variables from a Solidity function.
There was a problem hiding this comment.
@chrismaree That makes sense
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
This pr makes all return types consistent throughout the DVM by always first declaring a local variable and then returning the instance of this variable.
Note that functions that return more than 1 variable will use the named return variable standard such that implementations of contracts can read variables directly from contract reads.