Closed Janther closed 6 years ago
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.
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.
@Janther Yes, this is probably an unlikely situation to have more then 10 addresses per user, so I think we can close this issue
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 theUser
will have an array ofbytes32
of every physical address associated.This should open new possibilities like:
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.