Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specify return variable names for functions with public interfaces #273

Merged
merged 3 commits into from
Jul 10, 2018

Conversation

JamesLefrere
Copy link
Contributor

This PR makes some alterations to the interface contracts in order to specify the names of all function returns. One benefit of this is that the ABI now receives the return variable names, which makes making sense of returned values simpler.

The difference in the truffle artifact generated looks like this:

screen shot 2018-07-09 at 12 02 18

Other changes

  • NatSpec @return entries now reference the relevant variable name
  • Remove a NatSpec return variable which does not exist in the underlying function for the interface contract (getRewardPayoutInfo)
  • startNextRewardPayout appears to have no return value; remove this from the interface

Copy link
Contributor

@elenadimitrova elenadimitrova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a little twitchy about the underlying contracts not being updated to use the same return declarations but that is probably ok

/// @return amount Total amount of tokens taken aside for reward payout
/// @return tokenAddress Token address
/// @return blockTimestamp Block number at the time of creation
function getRewardPayoutInfo(uint256 _payoutId) public view returns (bytes32 reputationState, uint256 totalTokens, uint256 amount, address tokenAddress, uint256 blockTimestamp);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand you think this is unused but eventually when we come to returning the entire RewardPayoutCycle struct, that will return all 5 members regardless and reverting back to it will be a breaking change. Let's keep it as-is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that makes sense; I'll rebase and skip this commit 👍

@JamesLefrere JamesLefrere force-pushed the maintenance/add-return-variable-names branch from 1c83522 to 2305278 Compare July 9, 2018 13:36
@JamesLefrere
Copy link
Contributor Author

I was a little twitchy about the underlying contracts not being updated to use the same return declarations but that is probably ok

Yes, I considered whether that would be necessary or useful, but perhaps it's more work than it's worth, unless we enforce it globally with linting for example.

@JamesLefrere
Copy link
Contributor Author

JamesLefrere commented Jul 9, 2018

It seems like getRewardPayoutInfo was just changed in #256 - see f7f0b10#diff-181fc6eef83c50f44d61b3e11baf76a9R454

I'll rebase onto this (so we won't have the remaining token amount entry).

@JamesLefrere JamesLefrere force-pushed the maintenance/add-return-variable-names branch 2 times, most recently from 033b4c7 to df31d2c Compare July 10, 2018 10:07
* `startNextRewardPayout` appears to have no return value; correct the interface signature
* Add names for every return variable for the interface contracts
* Specify variable names for every NatSpec `@return` entry (for the interface contracts)
@elenadimitrova elenadimitrova merged commit 2a54255 into develop Jul 10, 2018
@elenadimitrova elenadimitrova deleted the maintenance/add-return-variable-names branch July 10, 2018 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants