[fast_exits] Pay exit bond to exit initiator #638

souradeep-das · 2020-06-03T07:16:10Z

Closes #629

pik694

Minor remarks 🙂

pik694 · 2020-06-03T11:10:25Z

plasma_framework/contracts/poc/fast_exits/Liquidity.sol

@@ -21,6 +21,8 @@ contract Liquidity is ERC721Full {

    struct ExitData {
        uint256 exitBondSize;
+        address exitInitiator;
+        bool bondReturned;


instead of having such a flag we could set exitInitiator to address(0)

yes @pik694, but in that case we wouldn't be able to differentiate between a wrong/invalid exitId provided and an already claimed bond in case we wan't to throw distinct messages for the users? 🤔

but should we need that? it is just an invalid claim

Lets remove it if we dont need to give out the specific reason(error message). Your call 😄

pik694 · 2020-06-03T11:11:36Z

plasma_framework/contracts/poc/fast_exits/Liquidity.sol

+    */
+    function getExitBond(uint160 exitId) public {
+        require(exitData[exitId].bondReturned == false, "Exit Bond Already returned");
+        require(exitData[exitId].exitInitiator != address(0), "ExitId is invalid");


I believe that this check is not necessary as the check blow is essential

pik694 · 2020-06-03T11:26:02Z

plasma_framework/contracts/poc/fast_exits/Liquidity.sol

+        PaymentExitDataModel.StandardExit[] memory exits = paymentExitGame.standardExits(
+            exitIdList
+        );
+        if (exits[0].utxoPos == 0) {


maybe assert here. this would eliminate need to use if else structure

ohh if we use assert here, we cant return back an error message? Do we want that? also require in the middle might not be a good idea?

require in the middle might not be a good idea?

I don't think it matters. And if-else + revert is really just the same thing.

ah...I know we are still handling the exit processed check in another issue.
However, can we put exit.utxoPos == 0 into a function now (and the part in getWithdraw too) with naming like: (eg.) checkExitProcessed so make this a bit easier to read.

And if-else + revert is really just the same thing

Thats true! I'll change to require for better readability then
Also having a function here to checkExitProcessed will make it simpler to make changes later

boolafish · 2020-06-05T09:49:23Z

plasma_framework/contracts/poc/fast_exits/Liquidity.sol

+        PaymentExitDataModel.StandardExit[] memory exits = paymentExitGame.standardExits(
+            exitIdList
+        );
+        if (exits[0].utxoPos == 0) {


require in the middle might not be a good idea?

I don't think it matters. And if-else + revert is really just the same thing.

boolafish · 2020-06-05T09:54:55Z

plasma_framework/contracts/poc/fast_exits/Liquidity.sol

+        PaymentExitDataModel.StandardExit[] memory exits = paymentExitGame.standardExits(
+            exitIdList
+        );
+        if (exits[0].utxoPos == 0) {


ah...I know we are still handling the exit processed check in another issue.
However, can we put exit.utxoPos == 0 into a function now (and the part in getWithdraw too) with naming like: (eg.) checkExitProcessed so make this a bit easier to read.

boolafish

lol bad timing for this comment and not give this earlier, would suggest we rename getWithdrawal and getExitbond to be sth not start with get. It would be confused as a getter function potentially.

Probably getWithdrawal -> withdrawExit or retreiveProcessedExit and getExitBond -> withdrawExitBond, retreiveExitBond ?

Anyway approval given, if you'd like to make the change just ping me and I will approve again.

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

pik694

Just minor typos. Can be merged in some next PR, or now, or never - up to you.

pik694 · 2020-06-09T22:50:13Z

plasma_framework/contracts/poc/fast_exits/Liquidity.sol

    }

    /**
     * @dev Get Amount from contract after exit is processed - (to be updated)
     * @param exitId The exit id
    */
-    function getWithdrawal(uint160 exitId) public {
+    function withdrawExit(uint160 exitId) public {
        require(
            super.ownerOf(exitId) == msg.sender,
            "Only the NFT owner of the respective exit can get the withdrawal"


Suggested change

"Only the NFT owner of the respective exit can get the withdrawal"

"Only the NFT owner of the respective exit can withdraw"

pik694 · 2020-06-09T22:50:38Z

plasma_framework/contracts/poc/fast_exits/Liquidity.sol

+     * @param exitId The exit id
+    */
+    function withdrawExitBond(uint160 exitId) public {
+        require(exitData[exitId].exitInitiator != address(0), "Exit Bond does not exist or is already claimed");


Suggested change

require(exitData[exitId].exitInitiator != address(0), "Exit Bond does not exist or is already claimed");

require(exitData[exitId].exitInitiator != address(0), "Exit Bond does not exist or has already been claimed");

Thanks @pik694, doing it on the next one.

souradeep-das added 2 commits June 3, 2020 12:41

feat: add func to get bond back

1092977

style: lint fix

9914352

souradeep-das requested review from pik694, boolafish and kevsul June 3, 2020 07:25

pik694 previously approved these changes Jun 3, 2020

View reviewed changes

boolafish previously approved these changes Jun 5, 2020

View reviewed changes

refactor: seperate check for processed exit

5a4aa7e

souradeep-das dismissed stale reviews from boolafish and pik694 via 5a4aa7e June 5, 2020 16:33

souradeep-das requested review from pik694 and boolafish June 5, 2020 16:41

boolafish previously approved these changes Jun 9, 2020

View reviewed changes

refactor: rename withdraw funcs

1fb583e

souradeep-das dismissed boolafish’s stale review via 1fb583e June 9, 2020 08:19

boolafish previously approved these changes Jun 9, 2020

View reviewed changes

pik694 previously approved these changes Jun 9, 2020

View reviewed changes

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

tests: add tests to check correct retrieval of bond

c5d4e11

souradeep-das dismissed stale reviews from pik694 and boolafish via c5d4e11 June 9, 2020 16:06

refactor: use single var for updated bond

a31f4dd

souradeep-das requested a review from pik694 June 9, 2020 16:25

style: js lint

fb3505a

pik694 approved these changes Jun 9, 2020

View reviewed changes

boolafish approved these changes Jun 10, 2020

View reviewed changes

souradeep-das merged commit fef1f7a into master Jun 10, 2020

souradeep-das deleted the souradeep/fast_exits_bond branch June 10, 2020 05:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fast_exits] Pay exit bond to exit initiator #638

[fast_exits] Pay exit bond to exit initiator #638

souradeep-das commented Jun 3, 2020

pik694 left a comment •

edited

Loading

pik694 Jun 3, 2020

souradeep-das Jun 3, 2020

pik694 Jun 3, 2020

souradeep-das Jun 3, 2020 •

edited

Loading

pik694 Jun 3, 2020

pik694 Jun 3, 2020

souradeep-das Jun 3, 2020 •

edited

Loading

boolafish Jun 5, 2020

boolafish Jun 5, 2020

souradeep-das Jun 5, 2020

boolafish Jun 5, 2020

boolafish Jun 5, 2020

boolafish left a comment •

edited

Loading

pik694 left a comment

pik694 Jun 9, 2020

pik694 Jun 9, 2020

souradeep-das Jun 10, 2020

	"Only the NFT owner of the respective exit can get the withdrawal"
	"Only the NFT owner of the respective exit can withdraw"

	require(exitData[exitId].exitInitiator != address(0), "Exit Bond does not exist or is already claimed");
	require(exitData[exitId].exitInitiator != address(0), "Exit Bond does not exist or has already been claimed");

[fast_exits] Pay exit bond to exit initiator #638

[fast_exits] Pay exit bond to exit initiator #638

Conversation

souradeep-das commented Jun 3, 2020

pik694 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

souradeep-das Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

souradeep-das Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boolafish left a comment • edited Loading

Choose a reason for hiding this comment

pik694 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pik694 left a comment •

edited

Loading

souradeep-das Jun 3, 2020 •

edited

Loading

souradeep-das Jun 3, 2020 •

edited

Loading

boolafish left a comment •

edited

Loading