strange-labs-uk / ethereum-raffle

Simple raffle style lottery DApp
7 stars 2 forks source link

Split code and data contracts #30

Open binocarlos opened 6 years ago

binocarlos commented 6 years ago

Currently - if we want to make kind of change to the contract code - we must deploy a new contract which will get a new address.

The history of all games before this point in time are part of the storage of the old contract and effectively lost.

Consider having a stable storage contract with a much simpler key:value store - this would need:

tasks

key value store contract

This must be kept as simple as possible as it essentially can never change. It should keep all state variables for all games.

This will mean we need to re-factor the data structure. Rather than save structs into an array - we would devise our own key format.

For example - the ticket price of the current game (e.g. index 5) is currently accessed as:

const price = games[5].settings.price

We would change this to be:

const key = `game.5.price`
const price = keyValueStoreContract.load(key, 'uint')

deal with types

The underlying key value store should be the following storage variable:

mapping (bytes32 => bytes32) database;

The limit for the length of a key is 32 chars.

We need to replicate the following structs:

struct GameSettings
{
  uint256 price;
  uint feePercent;
  uint start;
  uint end;
  uint minPlayers;
  uint complete;
  uint drawPeriod;
}

struct GameSecurity
{
  bytes32 entropy;
  bytes32 lastBlockHash;
  bytes32 secretKeyHash;
  string secretKey;
}

struct GameResults
{
  bool refunded;
  address winner;
  uint256 prizePaid;
  uint256 feesPaid;
}

We can save all of these as bytes32 and cast them in the main contract. Here is how we cast:

address will need their own storage variable so we need more than one database:

protecting access

The onlyRaffleContract accessor will check that the caller is the currently configured raffle address.

Only the owner of the storage contract can update the raffleContractAddress property by calling the updateRaffleContractAddress function.

This should be done as part of the migration of a new raffle contract.

deployment

Sequence of events:

When a new raffle contract is migrated - the migration script must update the address in the key-value store, otherwise the raffle contract will have no access to the database.

discovery

The raffle contract will need a keyValueStoreAddress property that can only be updated by the owner. We need to pass the key-value contract address to the constructor which will update the keyValueStoreAddress.

sketch of storage contract

contract KeyValueStore is Ownable {

  struct DatabaseStorage
  {
    // where we save bytes32 values
    mapping (bytes32 => bytes32) bytes32Values;

    // where we save address values
    mapping (bytes32 => address) addressValues;
  }

  address raffleContractAddress;
  DatabaseStorage database;

  modifier onlyRaffleContract() {
    require(msg.sender == raffleContractAddress);
    _;
  }

  function KeyValueStore() public Ownable() {

  }

  function updateRaffleContractAddress(address newAddress)
    public
    onlyOwner
    returns (address)
  {
    raffleContractAddress = msg.sender;
  }

  function setBytes32(bytes32 key, bytes32 value)
    public
    onlyRaffleContract
    returns (bytes32)
  {
    database.bytes32Values[key] = value;
    return value;
  }

  function setAddress(bytes32 key, address value)
    public
    onlyRaffleContract
    returns (address)
  {
    database.addressValues[key] = value;
    return value;
  }

  function getBytes32(bytes32 key, bytes32 value)
    public
    onlyRaffleContract
    returns (bytes32)
  {
    return database.bytes32Values[key];
  }

  function getAddress(bytes32 key, bytes32 value)
    public
    onlyRaffleContract
    returns (bytes32)
  {
    return database.addressValues[key];
  }
}
brtkwr commented 6 years ago

I think this is nice to have but hopefully once we iterate over a few times, we won't need to make any more changes to the actual contract.

We are basically reinventing a database here on blockchain :)

Also, we are promising to the user that for the sets of games stored within this contract, these were the sets of rules that applied.

New rules = New contract.

Also I don't see a huge benefit in keeping a record of all games that have ever been played other than the fact that it minimises our cost of creating a new game since: deploying a new contract is more expensive than creating a new game.

binocarlos commented 6 years ago

I'm thinking more about a critical bug is found type fixes and that we don't want to loose the history of previous games when making any changes.

I think this is nice to have but hopefully once we iterate over a few times, we won't need to make any more changes to the actual contract.

I think this is optimistic :)

Also, we are promising to the user that for the sets of games stored within this contract, these were the sets of rules that applied.

Yes I like that - the main focus is of doing this is to make sure we don't loose the history of previous games. I suppose we could keep a list of previously deployed contracts addresses and iterate over them asking for the history from each one.

@brtknr good feedback - what do we reckon - my instinct is keeping the state inside the same contract as the code will bite us at some point - what does everyone else think?

binocarlos commented 6 years ago

Another thing is that we have save the contract address of a game against the state of that game so anyone can see the contract code that ran that particular game.

Bear in mind that if we get this setup working well - we can scale out to have many other games. I was looking at the national lottery scratch card booth in Tescos earlier thinking - man we could implement all of these :-)

px3l commented 6 years ago

my position is always, keep state separate from function.

I think as well it is important to hold a full and complete history of games for stats and transparency purposes. Though this is more a marketing standpoint.

px3l commented 6 years ago

I think this is nice to have but hopefully once we iterate over a few times, we won't need to make any more changes to the actual contract.

If this is a successful app, i think we will want to revisit the code continually

px3l commented 6 years ago

how difficult is it to, rather than do this, if we need a significant rewrite we can point to the old address for previous history? Is there any other need for the previous history than marketing/stats?

ie.


------------------                ------------------  
|   codebase    |     --failed-->  |   codebase     | 
|        v1     |                 |        v2       |
------------------                ------------------
           ^                       |
           |________link __________|

or even just create a new contract that contains a database of duplicate data from the deployed v1, v2, v3 etc. so rather than split the state and function, we just duplicate the state somewhere easy to read just what we want. Remind me though, what is it exactly that we want from the history?

px3l commented 6 years ago

and actually now i have read through this, i also agree with

New rules = New contract.

I think we need a history for marketing and for our transparency stats. But I don't think we need to worry if a huge bug got found or something. we would just close the games in that contract and deploy a new one?