[Process Exit Bounty] Add process exit bounty for SE #665

souradeep-das · 2020-07-28T19:29:49Z

Overview

This PR adds the PE bounty for Standard Exits only

Out of scope for this PR

Make the bounty updatable
anti-Front running fix

pgebal

We do not have ExitBounty lib unit tests. I know it's tested implicitly in all the other usecases, but maybe we should have a unit test checking if it returns a correct value.

plasma_framework/contracts/src/exits/utils/ExitBounty.sol

plasma_framework/test/endToEndTests/FastExit.e2e.test.js

plasma_framework/test/endToEndTests/PaymentStandardExit.e2e.test.js

souradeep-das · 2020-08-07T17:42:40Z

@pgebal The python tests could have worked without changes, ( by assuming the process exit bounty to have been 0), but i have modified them to account for the bounty while starting a standard exit.
Please let me know if we should scrap the changes to the python tests, and move with the assumption of 0 bounty for them or if we feel the need to add any other py tests.( cc @boolafish )

boolafish

Haven't read the test yet, but are we still going to implement the upgrade for bounty?
#658 (comment)

plasma_framework/contracts/poc/fast_exits/Liquidity.sol

plasma_framework/contracts/src/exits/payment/PaymentExitGame.sol

plasma_framework/contracts/src/framework/ExitGameController.sol

souradeep-das · 2020-08-11T09:33:53Z

@boolafish Yes, I have the "make bounty updatable" and "anti- front running" on our board. Planning to do both of them together at the end after i land the IFE bounty PR

pgebal · 2020-08-11T15:00:58Z

@boolafish Yes, I have the "make bounty updatable" and "anti- front running" on our board. Planning to do both of them together at the end after i land the IFE bounty PR

Do you have an idea how to prevent front running? I'm asking, because if we come up with it later it may require a very major refactor, that could for example revert this PR in a big way.

souradeep-das · 2020-08-11T15:07:48Z

Mostly using the sender address to prevent simple front-running by the bots was the idea. Similar to how we do for the challenge exits. I guess an advanced anti-front running could be useful in the future ( which could prevent against dedicated attacks ), which would also require changes to the existing pattern we have for challenge exits along with process exits?

pgebal

Looks good, please fix the minor in python tests before merging.

plasma_framework/python_tests/tests/contracts/root_chain/test_process_exits.py

pgebal · 2020-08-11T16:42:51Z

@pgebal The python tests could have worked without changes, ( by assuming the process exit bounty to have been 0), but i have modified them to account for the bounty while starting a standard exit.
Please let me know if we should scrap the changes to the python tests, and move with the assumption of 0 bounty for them or if we feel the need to add any other py tests.( cc @boolafish )

I don't think we need more python tests.

pgebal · 2020-08-11T16:45:03Z

@boolafish I think we should have a requirement for 2 reviewers to approve a PR before merging. I think from all our repos that's the one that deserves it the most. What do you think?

boolafish

generally LGTM. have some comments.

plasma_framework/contracts/src/exits/payment/PaymentExitGame.sol

boolafish · 2020-08-12T02:35:59Z

plasma_framework/contracts/src/exits/utils/ExitBounty.sol

+     * 107000 is the approx gas usage for calling processExit()
+     */
+    function processStandardExitBountySize() internal view returns (uint256) {
+        return 107000 * tx.gasprice;


As for this implementation and design, it might be better to pass in gas price as args instead of using tx.gasprice for easier testing. I was actually surprise you can call a view function and pass in the gas price in the call to the Wrapper contract 😅 as it is not consuming any gas.

However, if we are adding upgrade mechanism, I think it will be easier to be the same as what we do for bond. So we don't use tx.gasprice to do the calculation but hardcode one price there.

Minor: slightly concern the abstraction might break a bit as ExitBounty quite likely would need a storage when introducing upgrade mechanism. But would leave that as out of scope just a warning this seems to break in the future.

passing gas price in args will indeed be better 👍 .
For the upgrade mechanism, i was thinking more in lines of updating the multiplier "K" instead of a new value for the size, so we would not need to update the bounty size frequently and can accommodate frequent gas changes? What do you think?

plasma_framework/contracts/mocks/exits/utils/ExitBountyWrapper.sol

plasma_framework/python_tests/tests/tests_utils/plasma_framework.py

plasma_framework/test/src/exits/payment/controllers/PaymentProcessStandardExit.test.js

boolafish · 2020-08-12T02:54:52Z

I think we should have a requirement for 2 reviewers to approve a PR before merging. I think from all our repos that's the one that deserves it the most. What do you think?

lol okay....if other people feel like having more reviewers for approval I will change the setup then. Was thinking to loosen it for faster development speed for v2.

pgebal · 2020-08-12T07:14:00Z

I think we should have a requirement for 2 reviewers to approve a PR before merging. I think from all our repos that's the one that deserves it the most. What do you think?

lol okay....if other people feel like having more reviewers for approval I will change the setup then. Was thinking to loosen it for faster development speed for v2.

Maybe I'm wrong on that. That was just an idea. If you think 1 approval is enough, then let's stay with 1.

boolafish · 2020-08-12T14:11:56Z

Just a reminder, we probably want to start a doc to track the breaking changes on V2.

I doubt you will implement this all the way to childchain, watcher, omg-js...etc so probably we should go back to @InoMurko's suggestion of documenting the integration changes.

This reverts commit e37d1e2.

…697) This reverts commit e37d1e2.

souradeep-das added 8 commits July 29, 2020 00:58

feat: add process exit bounty for SE

853c3be

fix: pass in msg.sender to exitgame

0741796

style: lint js

28c8afe

fix: coverage

89fefef

revert package.json

b48c204

merge v2.0.0 on branch

18dc9c9

fix: modify LC tests

ad65388

style: add newline eof

a4c45bb

souradeep-das marked this pull request as ready for review July 31, 2020 12:28

souradeep-das requested review from pgebal, boolafish and pnowosie July 31, 2020 12:29

Merge branch 'v2.0.0' into souradeep/exit_bounty

904bb4a

pgebal suggested changes Aug 3, 2020

View reviewed changes

plasma_framework/contracts/src/exits/utils/ExitBounty.sol Outdated Show resolved Hide resolved

plasma_framework/test/endToEndTests/FastExit.e2e.test.js Outdated Show resolved Hide resolved

plasma_framework/test/endToEndTests/PaymentStandardExit.e2e.test.js Outdated Show resolved Hide resolved

souradeep-das added 13 commits August 6, 2020 17:33

fix: modify python tests

50e4686

merge v2.0.0 on branch

51e3eff

fix: rem gasprice form py tests

e5d620a

fix: revert plasma_framework.py

bd33161

fix: revert plasma_framework.py

ba78534

feat: add Bounty lib tests

d75f2ff

style: js lint

f0e7077

try python tests

e3b527a

add to python tests

04af275

add w3 python tests

9ea8faa

fix: py tests

93ea584

fix: lint py

2d2b5c6

fix: lint py

6c78825

souradeep-das requested a review from pgebal August 7, 2020 17:43

boolafish reviewed Aug 11, 2020

View reviewed changes

plasma_framework/contracts/poc/fast_exits/Liquidity.sol Show resolved Hide resolved

plasma_framework/contracts/src/exits/payment/PaymentExitGame.sol Show resolved Hide resolved

plasma_framework/contracts/src/framework/ExitGameController.sol Show resolved Hide resolved

pgebal approved these changes Aug 11, 2020

View reviewed changes

plasma_framework/python_tests/tests/contracts/root_chain/test_process_exits.py Outdated Show resolved Hide resolved

fix: minor remove unused w3

95ee8d7

boolafish reviewed Aug 12, 2020

View reviewed changes

souradeep-das added 3 commits August 12, 2020 18:24

style: use gasprice as args in lib

5d63ef8

style: add eof line

8be513c

style: remove unused contract

bb49316

souradeep-das requested a review from boolafish August 12, 2020 13:43

boolafish approved these changes Aug 12, 2020

View reviewed changes

souradeep-das merged commit e37d1e2 into v2.0.0 Aug 12, 2020

souradeep-das deleted the souradeep/exit_bounty branch August 12, 2020 14:53

souradeep-das added a commit that referenced this pull request Aug 24, 2020

Revert "[Process Exit Bounty] Add process exit bounty for SE (#665)"

c3ed693

This reverts commit e37d1e2.

souradeep-das mentioned this pull request Aug 24, 2020

Revert "[Process Exit Bounty] Add process exit bounty for SE" #697

Merged

souradeep-das added a commit that referenced this pull request Aug 24, 2020

Revert "[Process Exit Bounty] Add process exit bounty for SE (#665)" (#…

2041786

…697) This reverts commit e37d1e2.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Process Exit Bounty] Add process exit bounty for SE #665

[Process Exit Bounty] Add process exit bounty for SE #665

souradeep-das commented Jul 28, 2020 •

edited

Loading

pgebal left a comment

souradeep-das commented Aug 7, 2020 •

edited

Loading

boolafish left a comment

souradeep-das commented Aug 11, 2020

pgebal commented Aug 11, 2020

souradeep-das commented Aug 11, 2020 •

edited

Loading

pgebal left a comment

pgebal commented Aug 11, 2020

pgebal commented Aug 11, 2020

boolafish left a comment

boolafish Aug 12, 2020 •

edited

Loading

souradeep-das Aug 12, 2020

boolafish commented Aug 12, 2020

pgebal commented Aug 12, 2020

boolafish commented Aug 12, 2020

[Process Exit Bounty] Add process exit bounty for SE #665

[Process Exit Bounty] Add process exit bounty for SE #665

Conversation

souradeep-das commented Jul 28, 2020 • edited Loading

Overview

Out of scope for this PR

pgebal left a comment

Choose a reason for hiding this comment

souradeep-das commented Aug 7, 2020 • edited Loading

boolafish left a comment

Choose a reason for hiding this comment

souradeep-das commented Aug 11, 2020

pgebal commented Aug 11, 2020

souradeep-das commented Aug 11, 2020 • edited Loading

pgebal left a comment

Choose a reason for hiding this comment

pgebal commented Aug 11, 2020

pgebal commented Aug 11, 2020

boolafish left a comment

Choose a reason for hiding this comment

boolafish Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

souradeep-das Aug 12, 2020

Choose a reason for hiding this comment

boolafish commented Aug 12, 2020

pgebal commented Aug 12, 2020

boolafish commented Aug 12, 2020

souradeep-das commented Jul 28, 2020 •

edited

Loading

souradeep-das commented Aug 7, 2020 •

edited

Loading

souradeep-das commented Aug 11, 2020 •

edited

Loading

boolafish Aug 12, 2020 •

edited

Loading