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

Cooperative settlement #1474

Merged
merged 14 commits into from
Jul 15, 2021
Merged

Cooperative settlement #1474

merged 14 commits into from
Jul 15, 2021

Conversation

palango
Copy link
Contributor

@palango palango commented Jun 22, 2021

What this PR does

Implement cooperative settle (#1383)

Tasks

  • Add check (+ test) for withdraw of to small amount (settle withdraw < deposit - withdraws)
  • Fix event data, adapt tests (test_cooperative_settle_channel_event)
  • Add gas estimates for cooperative settlement
  • Fix event checker to be able ti handle multiple events of same kind

Why I'm making this PR

What's tricky about this PR (if any)


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
    • if the PR adds or removes require() or assert()
      • add an entry in Changelog
      • open an issue in the client, light client, service repos so the change is reflected there
      • Just adding a message in require() doesn't require these steps.
  • 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.

@palango palango changed the title initial implementation WIP: Cooperative settle Jun 22, 2021
@codecov
Copy link

codecov bot commented Jun 24, 2021

Codecov Report

Merging #1474 (7ddc3cb) into master (2bfc938) will increase coverage by 0.15%.
The diff coverage is 100.00%.

❗ Current head 7ddc3cb differs from pull request most recent head f605324. Consider uploading reports for the commit f605324 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1474      +/-   ##
==========================================
+ Coverage   80.66%   80.81%   +0.15%     
==========================================
  Files          22       22              
  Lines        1562     1564       +2     
  Branches      190      190              
==========================================
+ Hits         1260     1264       +4     
+ Misses        255      253       -2     
  Partials       47       47              
Impacted Files Coverage Δ
raiden_contracts/utils/proofs.py 97.36% <ø> (+4.68%) ⬆️
raiden_contracts/utils/events.py 92.00% <100.00%> (+0.57%) ⬆️

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 738d126...f605324. Read the comment docs.

@palango palango force-pushed the coop-settle branch 5 times, most recently from 1a1cfb5 to 8ea8c7d Compare June 28, 2021 12:58
@palango palango force-pushed the coop-settle branch 4 times, most recently from f3ceef8 to fa40dc1 Compare June 29, 2021 15:02
@palango palango changed the title WIP: Cooperative settle Cooperative settlement Jun 29, 2021
@palango palango force-pushed the coop-settle branch 2 times, most recently from 327969c to 7bdb530 Compare June 29, 2021 15:54
@palango palango marked this pull request as ready for review June 29, 2021 15:55
@auto-assign auto-assign bot requested a review from konradkonrad June 29, 2021 15:55
@palango palango force-pushed the coop-settle branch 4 times, most recently from 9a28b87 to 7ba6598 Compare June 30, 2021 12:41
Copy link
Contributor

@karlb karlb left a comment

Choose a reason for hiding this comment

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

Really nice to finally have a coop settle after such a long time!

raiden_contracts/data/source/raiden/TokenNetwork.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@andrevmatos andrevmatos left a comment

Choose a reason for hiding this comment

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

I'm no solidity expert, but this looks good to me. It'd be good to have someone else more into solidity itself to review it though. I like the approach to leverage the double-withdraw signed structures, but we really need to optimise those gas usages

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.

3 participants