Skip to content
This repository has been archived by the owner on Jun 29, 2020. It is now read-only.

(Refactor / Improvement) Using the keccak for comparing inputs #77

Closed
Janther opened this issue Mar 7, 2018 · 7 comments · Fixed by #84
Closed

(Refactor / Improvement) Using the keccak for comparing inputs #77

Janther opened this issue Mar 7, 2018 · 7 comments · Fixed by #84

Comments

@Janther
Copy link
Contributor

Janther commented Mar 7, 2018

On the lookup by physical address

if (
       str_eq(users[wallet].physical_addresses[ai].country, country)
    && str_eq(users[wallet].physical_addresses[ai].state, state)
    && str_eq(users[wallet].physical_addresses[ai].city, city)
    && str_eq(users[wallet].physical_addresses[ai].location, location)
    && str_eq(users[wallet].physical_addresses[ai].zip, zip)
)

Instead of looping over every character on the string comparison for every input, the system could benefit from hashing the keccak of the attributes when storing it and using it for comparison.

if (users[wallet].physical_addresses[ai].keccak == keccak256(country, state, city, location, zip))

The keccak could also be used as an index or a proof of the existence of an address.

mapping (bytes32 => bool) addressExists;
@phahulin
Copy link
Contributor

phahulin commented Mar 9, 2018

@Janther could you elaborate on

keccak could also be used as an index or a proof of the existence of an address.

At the present moment we store addresses for each wallet as an array - this is probably not the best way. But the advantage of this is that we can find all addresses linked to the wallet. Do you propose we store an additional mapping of keccak to index in addresses array?

@Janther
Copy link
Contributor Author

Janther commented Mar 12, 2018

Yes mainly something like this:

contract ProofOfPhysicalAddress {
    mapping (bytes32 => bool) public addressExists;

    function register_address(
        string name,
        string country,
        string state,
        string city, string location, string zip,
        uint256 price_wei,
        bytes32 confirmation_code_sha3, uint8 sig_v, bytes32 sig_r, bytes32 sig_s)
    public payable
    {
        // ... normal register_address logic plus following line ... //
        addressExists[keccak256(name, country, state, city, location, zip)] = true;
    }
}

Then you can easily ask if an address has been registered. This is just an idea since the important part here is that we agree that keccak256(name, country, state, city, location, zip) is a proper way to create a unique identifier for an address so we can evaluate or use it as an index for mappings that would serve as shortcuts.

@phahulin
Copy link
Contributor

Yes, we can agree that using keccak256 as a unique identifier and for searching should be done.

I'm trying to understand if there is any benefit to store both array of addresses and an additional mapping of addresses. But this discussion should probably go to #79

@Janther
Copy link
Contributor Author

Janther commented Mar 12, 2018

In the case of having a mapping of Users containing an array of addresses, it makes sense to have a mapping of booleans just to show that an address does exist without having to go through the Users.
If we consider #79 then it makes no sense since the addresses are already accessible with a single call.

@phahulin
Copy link
Contributor

phahulin commented Mar 13, 2018

@pablofullana can I start working on the part that me and @Janther agreed on? that is

Instead of looping over every character on the string comparison for every input, the system could benefit from hashing the keccak of the attributes when storing it and using it for comparison.

I think it will improve the code to a large extent.

@pablofullana
Copy link

Works for me. @vbaranov any concerns on your side?

@vbaranov
Copy link

no, I have no concerns about it

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants