Implementing path predicates and rewards #292

kellan-simiotics · 2023-04-21T16:24:47Z

Changes

How to test these changes?

Related issues

…g that several different pool ids can be received as rewards for different stages/paths in the same session. .

And hooked it up to Python CLI and tests.

Hooked it up to `stakeTokensIntoSession`.

…dle both staking predicates and path choice predicates. Tested get/set for path choice predicates. Still need to implment and test the predicate check on path choice.

zomglings

A few comments and questions. I need to look more closely at the new tests and review the Solidity changes with more care, too.

I plan to do so a little later today @kellan-simiotics . In the mean time can you please address my current comments?

zomglings · 2023-05-16T21:22:11Z

contracts/mechanics/garden-of-forking-paths/GardenOfForkingPaths.sol

@@ -63,7 +76,7 @@ library LibGOFP {
        uint256 numSessions;
        mapping(uint256 => Session) sessionById;
        // session => stage => stageReward
-        mapping(uint256 => mapping(uint256 => StageReward)) sessionStageReward;
+        mapping(uint256 => mapping(uint256 => Reward)) sessionStageReward;


I don't think this will be a problem, but we need to check if this change will corrupt data when we upgrade existing contracts.

The reason I think it won't is because of how struct storage positions are calculated: https://docs.soliditylang.org/en/v0.8.20/internals/layout_in_storage.html

zomglings · 2023-05-16T21:23:50Z

contracts/predicates/GOFPPredicates.sol

+        uint256 maxStakable,
+        address gofpAddress,
+        uint256 sessionId,
+        address player,


@kellan-simiotics : Can you create a .md file in this PR which discusses predicates, and explains our reasoning behind the signatures? We can pull content from our slack conversation about it.

zomglings · 2023-05-16T21:33:31Z

cli/enginecli/test_gofp.py

@@ -1,10 +1,13 @@
 import unittest


locust changes in this file:

Generated (from repo root) with:

locust -r . main HEAD -f json -o diffs.json

Top-level objects with changes in test_gofp.py:

jq '.locust[] | select(.file == "cli/enginecli/test_gofp.py") | .changes[].name' diffs.json

Returns:

"TestPlayerFlow" "TestAdminFlow" "TestCallbacks" "GOFPTestCase" "TestFullGames" "math" "web3" ".None.GOFPFacet" ".None.MockTerminus" ".None.MockErc20" ".None.MockERC721" ".None.GOFPPredicates"

(The .None. looks like a small bug in locust)

GOFPTestCase:

$ jq '.locust[] | select(.file == "cli/enginecli/test_gofp.py") | .changes[] | select(.name == "GOFPTestCase")' diffs.json { "name": "GOFPTestCase", "type": "class", "line": 284, "changed_lines": 21, "total_lines": 85, "children": [ { "name": "GOFPTestCase.setUpClass", "type": "function", "line": 286, "changed_lines": 21, "total_lines": 78, "children": [] }, { "name": ".None.GOFPPredicates", "type": "usage", "line": 302, "changed_lines": 1, "total_lines": 1, "children": [] }, { "name": ".None.GOFPPredicates.GOFPPredicates", "type": "usage", "line": 302, "changed_lines": 1, "total_lines": 1, "children": [] } ] }

It's just the deployment of predicates.

TestCallbacks:

$ jq '.locust[] | select(.file == "cli/enginecli/test_gofp.py") | .changes[] | select(.name == "TestCallbacks") | .children[] | select(.type == "function") | .name' diffs.json "TestCallbacks.test_session_staking_predicate" "TestCallbacks.test_path_choice_predicate"

These tests check that admins can set predicates, and that the predicate callbacks function as expected.

TestPlayerFlow:

$ jq '.locust[] | select(.file == "cli/enginecli/test_gofp.py") | .changes[] | select(.name == "TestPlayerFlow") | .children[] | select(.type == "function")' diffs.json { "name": "TestPlayerFlow.test_player_can_stake_and_unstake_nfts", "type": "function", "line": 1701, "changed_lines": 148, "total_lines": 148, "children": [] } { "name": "TestPlayerFlow.test_player_can_stake_into_free_session", "type": "function", "line": 1850, "changed_lines": 39, "total_lines": 79, "children": [] } { "name": "TestPlayerFlow.test_player_is_rewarded_for_making_a_choice_in_stages_that_have_rewards", "type": "function", "line": 2959, "changed_lines": 120, "total_lines": 140, "children": [] } { "name": "TestPlayerFlow.test_player_is_rewarded_for_choosing_path_that_has_reward", "type": "function", "line": 3100, "changed_lines": 187, "total_lines": 187, "children": [] } { "name": "TestPlayerFlow.test_player_can_receive_different_pool_ids_in_stage_and_path_rewards", "type": "function", "line": 3288, "changed_lines": 77, "total_lines": 148, "children": [] }

These method test path reward functionality, but not predicates.

TestAdminFlow:

$ jq '.locust[] | select(.file == "cli/enginecli/test_gofp.py") | .changes[] | select(.name == "TestAdminFlow") | .children[] | select(.type == "function")' diffs.json { "name": "TestAdminFlow.test_create_forgiving_session_then_get_session_active", "type": "function", "line": 468, "changed_lines": 1, "total_lines": 46, "children": [] } { "name": "TestAdminFlow.test_setting_same_stage_reward_twice_overwrites_first_value", "type": "function", "line": 1393, "changed_lines": 35, "total_lines": 58, "children": [] } { "name": "TestAdminFlow.test_set_and_get_path_rewards", "type": "function", "line": 1452, "changed_lines": 101, "total_lines": 121, "children": [] } { "name": "TestAdminFlow.test_non_game_master_cannot_set_path_rewards", "type": "function", "line": 1574, "changed_lines": 52, "total_lines": 52, "children": [] } { "name": "TestAdminFlow.test_setting_same_path_reward_twice_overwrites_first_value", "type": "function", "line": 1627, "changed_lines": 71, "total_lines": 71, "children": [] }

Nothing related to predicates (that stuff is in TestCallbacks). Tests reward flows for admins - pertinent to new path rewards.

zomglings · 2023-05-16T21:37:08Z

cli/enginecli/test_gofp.py

@@ -178,6 +181,50 @@
    "type": "event",
 }

+PATH_REWARD_CHANGED_ABI = {


Are we testing that an appropriate event gets fired when we set up a predicate?

zomglings · 2023-05-16T21:52:24Z

cli/enginecli/test_gofp.py

+
+
+class TestPlayerFlow(GOFPTestCase):
+    def test_player_can_stake_and_unstake_nfts(self):


Is this a new test introduced in this PR?

No, I think it's an error with the diff editor.

zomglings · 2023-05-16T21:52:49Z

cli/enginecli/test_gofp.py

+        for token_id in unstaked_token_ids:
+            self.assertEqual(self.nft.owner_of(token_id), self.player.address)
+
+    def test_player_can_stake_into_free_session(self):


Ditto, is this a new test introduced in this PR?

No, same as above.

zomglings · 2023-05-16T21:53:59Z

cli/enginecli/test_gofp.py

+            self.assertEqual(erc1155_transfer_event["args"]["id"], self.reward_pool_id)
+            self.assertEqual(
+                erc1155_transfer_event["args"]["value"],
+                (2**1) * (3**2),


Did we write this together?

It is clever, but it makes it hard to understand what's happening in the test. What do you think @kellan-simiotics ? Should we do it in a more dull but readable way?

Added a comment.

zomglings

lgtm

zomglings

lgtm

Setting path rewards for one stage.

9dfe570

kellan-simiotics requested a review from zomglings April 21, 2023 16:24

kellan-simiotics marked this pull request as draft April 21, 2023 16:24

kellan-simiotics changed the title ~~Setting path rewards for one stage.~~ Getting/setting path rewards Apr 24, 2023

kellan-simiotics changed the title ~~Getting/setting path rewards~~ getting/setting path rewards Apr 24, 2023

Kellan Wampler added 2 commits April 24, 2023 11:49

Adding tests for get/set path rewards.

0e824ef

Distibute path rewards when choosing path. Test rewards.

4ecab69

kellan-simiotics changed the title ~~getting/setting path rewards~~ Implementing path rewards Apr 24, 2023

Added testing of path rewards in full game tests.

d0ae785

kellan-simiotics marked this pull request as ready for review April 25, 2023 14:32

Kellan Wampler and others added 9 commits April 26, 2023 01:02

Adding tests for duplicate stage and path rewards. Added test checkin…

c5a8bb3

…g that several different pool ids can be received as rewards for different stages/paths in the same session. .

Fixing some test names.

a3b0cc4

Added GOFPPredicates library

c1a3eac

And hooked it up to Python CLI and tests.

Adding get/set staking predicate calldata.

69d1966

First (failed) attempt at calling predicates using Yul

a208391

Got callSessionStakingPredicate working

465c7bc

Hooked it up to `stakeTokensIntoSession`.

Cleaned up test code

accf7f1

Refactored predicate logic in gofp to a 'callPredicate' method to han…

ce245bd

…dle both staking predicates and path choice predicates. Tested get/set for path choice predicates. Still need to implment and test the predicate check on path choice.

Checking path choice predicate and tests for predicate.

85aa099

zomglings reviewed May 16, 2023

View reviewed changes

zomglings changed the title ~~Implementing path rewards~~ Implementing path predicates and rewards Jun 23, 2023

Kellan Wampler added 3 commits August 2, 2023 07:41

Merge branch 'main' into garden-v2

0fc7a1d

Adding events for staking and path choice predicates.

bf22054

Some information about GoFP predicates.

11a7584

zomglings approved these changes Aug 8, 2023

View reviewed changes

zomglings merged commit 643f269 into main Aug 8, 2023
1 check passed

zomglings deleted the garden-v2 branch August 8, 2023 21:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing path predicates and rewards #292

Implementing path predicates and rewards #292

kellan-simiotics commented Apr 21, 2023

zomglings left a comment

zomglings May 16, 2023

zomglings May 16, 2023

zomglings May 16, 2023

zomglings May 16, 2023

zomglings May 16, 2023

zomglings May 16, 2023

zomglings May 16, 2023

zomglings May 16, 2023

zomglings May 16, 2023

kellan-simiotics Aug 2, 2023

zomglings May 16, 2023

kellan-simiotics Aug 2, 2023

zomglings May 16, 2023

kellan-simiotics Aug 2, 2023

zomglings left a comment

zomglings left a comment



		class TestPlayerFlow(GOFPTestCase):
		def test_player_can_stake_and_unstake_nfts(self):

Implementing path predicates and rewards #292

Implementing path predicates and rewards #292

Conversation

kellan-simiotics commented Apr 21, 2023

Changes

How to test these changes?

Related issues

zomglings 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

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

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

zomglings left a comment

Choose a reason for hiding this comment

zomglings left a comment

Choose a reason for hiding this comment