Minimal EIP2535 implementation of Inventory #324

zomglings · 2023-07-31T06:50:52Z

This PR introduces a minimal EIP2535 implementation of an inventory into this repository. This implementation is adapted from the original implementation at https://github.com/G7DAO/contracts.

We have added this implementation here for two reasons:

Having an implementation in this repository makes our operations much simpler as we can access all our contracts using a single CLI. Previously, we were interacting with the inventory with the game7ctl CLI while all other contract interactions were done with the CLI in this repo.
We have made several simplifications to the Inventory contract that might break Game7's deployment pipelines. This is less painful for all involved.

Resolves #319 and #322

These were just there to generate the Python scripts, which will need to be updated to support the new Inventory anyway.

That file was only there to generate the CLI before. CLI will need to be updated, as will the setup-inventory script.

And organized it to be flatter (file structure).

As a part of this, I also added: 1. The ability to specify existing facets for diamond_gogogo and for inventory_gogogo. This lays the foundation to eventually resolve #315 2. A fix for #323

from https://github.com/G7DAO/contracts And I got the tests working.

`g7dao` -> `moonstreamdao`

It complicates scope and was only being used twice.

And made comments more accurate

The slot types are implicitly defined by the items that can be equipped in those slots. Having an explicit notion of slot type with no conceivable way to enforce it creates two sources of truth about what a slot does - it's type and the set of items equippable in that slot. This makes the system more complex and difficult to work with, with no additional benefit.

This required a negation in contract `_unequip` logic. I also changed the signature of `setSlotPersistent`. There are currently no tests for `setSlotPersistent`. TODO(zomglings): Write them!

Unnecessary.

No need to index the slot itself - that will only be emitted once. We are now indexing the persistence.

Also, unindexed `persistent` from `SlotCreated` event.

Removed `persistent` argument from the `SlotCreated` event - it is duplicated in the `NewSlotPersistence` emission.

... emission from `createSlot`.

class method in `InventoryTestCase`. This will make it easier to test other Inventory implementations in the future.

Also added `NewSlotPersistence` event emission from that method.

This is only one of the tests we need to write for this behavior

kellan-simiotics

I'm not sure if there is anything critical missing, but the contract is rather difficult to use if you can ask a slot for all of its equippable options. Ran into this difficulty during CG testing.

kellan-simiotics · 2023-07-31T14:01:59Z

contracts/inventory/InventoryFacet.sol

        slot.SlotURI = newSlotURI;
        istore.SlotData[slotId] = slot;
        emit NewSlotURI(slotId);
    }

-    function slotIsUnequippable(uint256 slotId) external view returns (bool) {
+    function slotIsPersistent(uint256 slotId) external view returns (bool) {


Are we attached to this name? I don't like IsUnequippable, but I don't like IsPersistent much better. Maybe we could pick a name with the same polarity... IsChangeable?

This name was suggested by Diniz from OP Games. He subclassed InventoryFacet for their usage.

It is also a name that people have responded well to.

I suggest we merge this PR in with Persistent as the name, and we can change it later based on research and feedback.

contracts/inventory/IInventory.sol

kellan-simiotics · 2023-07-31T14:49:37Z

contracts/inventory/InventoryFacet.sol

@@ -282,7 +234,14 @@ contract InventoryFacet is
        address itemAddress,
        uint256 itemPoolId,
        uint256 maxAmount
-    ) external onlyAdmin requireValidItemType(itemType) {
+    ) external onlyAdmin {


I think we need a way to see all of the equippable items for a slot. Am I missing a way we are exposing this?

I don't even know how to expose this on-chain without adding an enumeration.

That can be an extension of the base interface (see IERC721Enumerable, which is an extension of IERC721). I personally prefer to get that information from events. I don't think it's necessary to the system.

Yes, but contract administrators won't always have easy access to the events. Can't we just dump the SlotEligibleItems object?

cli/web3cli/test_inventory.py

kellan-simiotics · 2023-08-01T20:12:03Z

cli/web3cli/test_inventory.py

+        )
+
+    def test_player_cannot_unequip_erc20_tokens_from_persistent_slot_but_can_increase_amount(self):
+        """


I think we should change the test name to indicate the test is broken or just comment out the whole test for now. I read through equip / _unequip methods a couple times before I noticed the comment that noted this test was unfinished as the functionality was missing.

I renamed the test to test_player_cannot_unequip_erc20_tokens_from_persistent_slot.

It's still a useful test as it tests persistence of slots.

I was using an ERC20 token to test ERC721 behavior in the inventory. The tests asserted a `VirtualMachineError`, which could have been triggered by the incorrect use of an ERC20 token as an ERC721 token.

kellan-simiotics

lg

zomglings and others added 25 commits July 26, 2023 21:02

Updated .gitignore

3d9b86e

Removed old IInventory and LibInventory

63000ad

These were just there to generate the Python scripts, which will need to be updated to support the new Inventory anyway.

Removed old contracts/InventorFacet.sol

09c5e0f

That file was only there to generate the CLI before. CLI will need to be updated, as will the setup-inventory script.

Renamed enginecli -> web3cli

b5ccb1c

Moved over Inventory code from G7DAO/contracts

2664149

And organized it to be flatter (file structure).

Updated InventoryFacet comment

dee709f

Updated core.py to contain inventory_gogogo and related functionality

9022045

As a part of this, I also added: 1. The ability to specify existing facets for diamond_gogogo and for inventory_gogogo. This lays the foundation to eventually resolve #315 2. A fix for #323

Ported over InventoryFacet...

d155ed3

from https://github.com/G7DAO/contracts And I got the tests working.

Inventory Diamond storage slot...

ae699e2

`g7dao` -> `moonstreamdao`

onlyContractSubjectOwner -> onlySubjectTokenOwner

fb0323c

Removed requireValidItemType modifier

5041ead

It complicates scope and was only being used twice.

Removed all functionality related to subject slots

56bfa77

pragma solidity ^0.8.0;

bfe4910

And made comments more accurate

Merge branch 'inventory' of github.com:moonstream-to/web3 into inventory

56f7530

Nonunequippable -> persistent

decb662

This required a negation in contract `_unequip` logic. I also changed the signature of `setSlotPersistent`. There are currently no tests for `setSlotPersistent`. TODO(zomglings): Write them!

Removed "SlotID" field from Slot struct

23aeaad

Unnecessary.

Changed indexing on SlotCreated events

7ef0128

No need to index the slot itself - that will only be emitted once. We are now indexing the persistence.

Added NewSlotPersistence event

11323bc

Also, unindexed `persistent` from `SlotCreated` event.

createSlot fires NewSlotURI, NewSlotPersistence

a693c86

Removed `persistent` argument from the `SlotCreated` event - it is duplicated in the `NewSlotPersistence` emission.

IInventory - license

c331b01

ContractAddressDesignated -> NewSubjectAddress

9046a23

Added tests for NewSlotURI and NewSlotPersistence...

0c134ff

... emission from `createSlot`.

Factored Inventory deployment into separate...

b355204

class method in `InventoryTestCase`. This will make it easier to test other Inventory implementations in the future.

Tests for setSlotPersistent

fb0b3d2

Also added `NewSlotPersistence` event emission from that method.

zomglings requested a review from kellan-simiotics July 31, 2023 06:50

zomglings added 3 commits July 30, 2023 23:56

Added test for unequipping from persistent slot

fafb329

This is only one of the tests we need to write for this behavior

BROWNIE_LIB=1

2a1eff2

Updated chainlink version in brownie-config.yaml

dd38fe0

kellan-simiotics requested changes Aug 1, 2023

View reviewed changes

Neeraj Kashyap added 3 commits August 1, 2023 13:55

Fixed two tests based on @kellan-simiotics feedback

e2f90f9

I was using an ERC20 token to test ERC721 behavior in the inventory. The tests asserted a `VirtualMachineError`, which could have been triggered by the incorrect use of an ERC20 token as an ERC721 token.

Renamed test as per @kellan-simiotics suggestion

f38985f

Fixed (thank you @kellan-simiotics again)

5405532

kellan-simiotics approved these changes Aug 1, 2023

View reviewed changes

zomglings merged commit 5d12e3e into main Aug 1, 2023

zomglings deleted the inventory branch August 1, 2023 23:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimal EIP2535 implementation of Inventory #324

Minimal EIP2535 implementation of Inventory #324

zomglings commented Jul 31, 2023

kellan-simiotics left a comment

kellan-simiotics Jul 31, 2023

zomglings Aug 1, 2023

kellan-simiotics Aug 1, 2023

kellan-simiotics Jul 31, 2023

zomglings Aug 1, 2023

kellan-simiotics Aug 1, 2023

kellan-simiotics Aug 1, 2023

zomglings Aug 1, 2023

zomglings Aug 1, 2023

kellan-simiotics left a comment

Minimal EIP2535 implementation of Inventory #324

Minimal EIP2535 implementation of Inventory #324

Conversation

zomglings commented Jul 31, 2023

kellan-simiotics 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

kellan-simiotics left a comment

Choose a reason for hiding this comment