(Improvement / Refactor) proposal for storage schema

[Janther]

• What is it? (leave one option)

  • (Improvement) proposal (i.e. some existing feature can be improved)
  • (Refactor) proposal (i.e. no new features or updates, just code optimization/library updates)

• What feature are you talking about?

I've been tinkering with other possibilities to improve the current schema of users and arrays of addresses.
I propose to store the addresses in a mapping where the keys will be the result of keccak256(name, country, state, city, location, zip) of each address.
Maintaining the same behaviour the User will have an array of bytes32 of every physical address associated.

contract ProofOfPhysicalAddress {
    // Main structures:
    struct PhysicalAddress {
        string name;

        string country;
        string state;
        string city;
        string location;
        string zip;

        uint256 creation_block;
        bytes32 confirmation_code_sha3;
        uint256 confirmation_block;
    }

    struct User {
        uint256 creation_block;
        bytes32[] physical_addresses_indexes;
    }

    mapping (address => User) public users;
    mapping (address => PhysicalAddress) public physical_addresses;

    // Stats:
    uint64 public total_users;
    uint64 public total_addresses;
    uint64 public total_confirmed;
}

• What is proposed feature?

This should open new possibilities like:

• Having the User store an extra array of confirmed addresses.

    struct User {
        uint256 creation_block;
        bytes32[] physical_addresses_indexes;
        bytes32[] confirmed_physical_addresses_indexes;
    }

• removing the confirmation code from the Address structure and using it as a mapping:

contract ProofOfPhysicalAddress {
    struct PhysicalAddress {
        string name;

        string country;
        string state;
        string city;
        string location;
        string zip;

        uint256 creation_block;
        uint256 confirmation_block;
    }

    mapping (address => PhysicalAddress) public physical_addresses;

    // confirmation_code_sha3 => index of address
    mapping (bytes32 => bytes32) internal physical_addresses_indexes_by_confirmation_code;

    function is_address_confirmed(bytes32 index)
        public constant returns (bool)
    {
        return (physical_addresses[index].confirmation_block > 0);
    }

    function user_address_by_confirmation_code(
        address wallet,
        bytes32 confirmation_code_sha3
    ) public constant returns (bool, bytes32, bool) {
        require(user_exists(wallet));
        for (uint256 ai = 0; ai < users[wallet].physicalAddressIndexes.length; ai += 1) {
            bytes32 memory index = testUsers[wallet].physicalAddressIndexes[ai];
            if (index == physical_addresses_indexes_by_confirmation_code[confirmation_code_sha3]) {
                return (true, index, is_address_confirmed(index));
            }
        }
        return (false, 0x0, false);
    }

    function confirm_address(
        string confirmation_code_plain,
        uint8 sig_v,
        bytes32 sig_r,
        bytes32 sig_s
    ) public {
        // ... Normal logic for validating the message ... //
        bool found;
        bytes32 index; // this index is the index for the mapping
        bool confirmed;
        (found, index, confirmed) = user_address_by_confirmation_code(msg.sender, keccak256(confirmation_code_plain));
        require(found);

        if (confirmed) {
            revert();
        } else {
            physical_addresses[index].confirmation_block = block.number;
            total_confirmed += 1;
            delete physical_addresses_indexes_by_confirmation_code[keccak256(confirmation_code_plain)];
        }
    }
}   

• Additional information

These are only possibilities that can be made if we rearrange slightly the storing schema. We don't need to implement these but consider changing the schema.

[phahulin]

I think there are some typos in code examples that make things not very clear to me, please double check my understanding :)

I propose to store the addresses in a mapping where the keys will be the result of keccak256(name, country, state, city, location, zip) of each address.

on the other hand, there is a mapping from address:

...
        uint256 confirmation_block;
    }

    mapping (address => PhysicalAddress) public physical_addresses;
...

shouldn't it be a mapping from bytes32 ?

Later, a call to is_address_confirmed() is made:

    function user_address_by_confirmation_code(
...
            if (index == physical_addresses_indexes_by_confirmation_code[confirmation_code_sha3]) {
                return (true, index, is_address_confirmed(index));
            }
...

but if index != 0x0 => it's not confirmed, because during confirm_address() the corresponding entry is removed from physical_addresses_indexes_by_confirmation_code, so is_address_confirmed and the third value in return tuple looks redundant. Probably take physical_addresses_indexes_by_confirmation_code[confirmation_code_sha3] out of the loop and then use return (true, index);

Also, here

    function confirm_address(
...
        if (confirmed) {
            revert();
        } else {
            physical_addresses[index].confirmation_block = block.number;
            total_confirmed += 1;
            delete physical_addresses_indexes_by_confirmation_code[keccak256(confirmation_code_plain)];
        }
...

I think a step to add index to confirmed_physical_addresses_indexes is missing, should be

...
            delete physical_addresses_indexes_by_confirmation_code[keccak256(confirmation_code_plain)];
            confirmed_physical_addresses_indexes.push(index);
 ...

---

So, if I understand correctly, you're proposing to keep a separate global mapping of physical addresses' keccak256 => physical addresses, while users only store references to entries of that mapping, is that correct?

I like the idea of accessing an address in a single call without for loops, however, I'm not so sure about the global mapping, because it's not actually global in the sense that it's not shared by users. We have to assume that two users can't reference the same element of physical_addresses (because one of them can have it confirmed, and the other one - unconfirmed). Maybe we can have the same idea of keeping keccak256-based index of addresses, but store it for each individual user?

    struct User {
        uint256 creation_block;
        PhysicalAddress[] physical_addresses;
        mapping (bytes32 => uint256) physical_addresses_indexes;
    }

On the other hand, I don't think it's a realistic case when a user would confirm 10+ addresses, most probably < 10, so is there any advantage of introducing new structures compared to for loops? Maybe we should estimate gas usage in both cases.

[Janther]

Thank you for your answer @phahulin.

I completely missed the use case where the same physical person would own more than 1 address and would want to register all of them.

You are right on this one.

The User struct that you are proposing doesn't have a real benefit over the existing one however, I liked the push on confirmed physical addresses.

struct User {
    uint256 creation_block;
    PhysicalAddress[] physical_addresses;
    uint256[] confirmed_physical_addresses_indexes;
}

we don't need the keccak index if the universe is going to be less than 10.

[phahulin]

@Janther Yes, this is probably an unlikely situation to have more then 10 addresses per user, so I think we can close this issue