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

Balance proof as a namedtuple instead of a tuple #1272

Merged
merged 3 commits into from
Oct 16, 2019

Conversation

pirapira
Copy link
Contributor

@pirapira pirapira commented Oct 5, 2019

What this PR does

This PR changes create_balance_proof()s return value from a tuple to a dictionary.

Why I'm making this PR

  • accessing tuple elements like balance_proof[0] balance_proof[3] was less readable than balance_proof["balance_hash"].
  • in order to use **balance_proof in other function calls, balance_proof has to be a dictionary.

What's tricky about this PR (if any)

The fields of the dictionary have the same name as the parameters of create_balance_proof_countersignature().


Any reviewer can check these:

  • If the PR is fixing a bug or adding a feature, add an entry to CHANGELOG.md.
  • If the PR changed a Solidity source, run make compile_contracts and add the resulting raiden_contracts/data/contracts.json in the PR.
  • If the PR is changing documentation only, add [skip ci] in the commit message so Travis does not waste time.
    • But, if the PR changes comments in a Solidity source, do not add [skip ci] and let Travis check the hash of the source.
  • In Python, use keyword arguments
  • Squash unnecessary commits
  • Comment commits
  • Follow naming conventions
    • solidityFunction
    • _solidity_argument
    • solidity_variable
    • python_variable
    • PYTHON_CONSTANT
  • Follow the Signature Convention in CONTRIBUTING.md
  • For each new contract
    • The deployment script deploys the new contract.
    • etherscan_verify.py runs on the new contract.
  • Bookkeep
    • The gas cost of new functions are stored in gas.json.
  • Solidity specific conventions
    • Document arguments of functions in natspec
    • Care reentrancy problems
  • When you catch a require() failure in Solidity, look for a specific error message like pytest.raises(TransactionFailed, match="error message"):

And before "merge" all checkboxes have to be checked. If you find redundant points, remove them.

@pirapira pirapira force-pushed the balance-proof-as-dict branch from 61bb3ab to 08d5717 Compare October 7, 2019 08:26
@codecov
Copy link

codecov bot commented Oct 7, 2019

Codecov Report

Merging #1272 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1272   +/-   ##
=======================================
  Coverage   81.66%   81.66%           
=======================================
  Files          21       21           
  Lines        1456     1456           
  Branches      190      190           
=======================================
  Hits         1189     1189           
  Misses        228      228           
  Partials       39       39

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e66852f...f21cbf8. Read the comment docs.

instead of a tuple.

This gets rid of many balance_proof[0], balance_proof[1] etc.
Also, after this change, calls to create_balance_proof_countersignature()
can easily be turned into calls with keyword arguments.
@pirapira pirapira force-pushed the balance-proof-as-dict branch from 08d5717 to 05b4f48 Compare October 15, 2019 13:32
@pirapira pirapira requested a review from LefterisJP October 15, 2019 13:51
@LefterisJP
Copy link
Contributor

Hey @pirapira I will review the PR as requested but before I do I have a suggestion to tweak what this PR is doing. If you like it please implement it, if not then I will review the PR as it already is.

In the "Why I am making this PR" section you stated:

  1. accessing tuple elements like balance_proof[0] balance_proof[3] was less readable than balance_proof["balance_hash"].

I think that what's even more readable is a named tuple. But apart from readability named tuples are also very well typed and can be checked with mypy while dictionaries not so much unless we use the typed dicts.

I know this is tests-only code so that's why I am leaving it up to you to decide what to do.

So it would be something like

class BalanceProof(NamedTuple):
    balance_hash: TypeOfBalanceHash
    nonce: int
    additional_hash: TypeOfAddHash
    signature: TypeOfSignature
  1. in order to use **balance_proof in other function calls, balance_proof has to be a dictionary.

You can do that with named tuples too! You just need to unpack them as a dict. So if balance_proof is a named tuple it would be: **balance_proof._asdict().

@pirapira
Copy link
Contributor Author

@LefterisJP then I'll do namedtuple. I guess the additional change is not big.

@pirapira pirapira changed the title Balance proof as a dictionary instead of a tuple Balance proof as a namedtuple instead of a tuple Oct 15, 2019
Copy link
Contributor

@LefterisJP LefterisJP left a comment

Choose a reason for hiding this comment

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

thank you. LGTM

)
token_network.functions.updateNonClosingBalanceProof(
channel_identifier, A, B, *balance_proof_A, balance_proof_update_signature_B
channel_identifier,
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: add kwargs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be done here, after Alderaan: #1292

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm ... that's not what I mean. I meant to specify the name of the arguments such as

token_network.functions.updateNonClosingBalanceProof(
    channel_identifier=channel_identifier,
    participant1=A,
    ....
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Positional arguments cannot come after kwargs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I just noticed *balance_proof_A._asdict().values(). All right then, disregard.

@pirapira pirapira merged commit 7128e5a into raiden-network:master Oct 16, 2019
@pirapira pirapira deleted the balance-proof-as-dict branch October 16, 2019 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants