Is the event field indexedKey supposed to be indexed?

[davekaj]

The field from the event appears that it is supposed to be indexed, but it isn't:

resolvers/contracts/profiles/TextResolver.sol

Line 8 in f0ef9c9

event TextChanged(bytes32 indexed node, string indexedKey, string key);

Also, it appears it was indexed long ago in the past, as seen by this event:
https://etherscan.io/tx/0xf49facd89a265d0bc2a8ea79965b48dd577fbafc2e355905e41492e17656b086#eventlog

I can across then when updating the ENS subgraph - https://github.com/graphprotocol/ens-subgraph/tree/master-backup

Seems like the word indexed might have been removed when upgrading the contract?

[Arachnid]

Good catch. Looping in @ricmoo, who wrote https://eips.ethereum.org/EIPS/eip-634.

One problem with indexing the keys here is that you can't index variable length types; if you specify string indexed in an event, Solidity hashes it, making it impossible to recover the plaintext.

[ricmoo]

Oh, that is why it was both indexed and repeated as a non-indexed, so it can be filtered and fetched. Although, wondering now if that is still the best course of action?

I will take a look into this shortly. :)

[davekaj]

The tricky part for changing the graph node is that these two events have the exact same signature, but because one string is indexed, the event logs have different data types. And since we are looking for all resolver events on any contracts, both indexed and non-indexed versions can be out there.

A rare occurrence, but nonetheless something we need to support. We will have to look at event signatures, while also checking the ABI for any variable length types that are indexed, and then treat them as bytes32.

[Arachnid]

@ricmoo In this case I don't think it's the same parameter - the first argument is the namehash, the second is the text field.

@davekaj Good point. If we change the event, then, we should change the name as well.

[ricmoo]

Here was the original commit, which passes the key in twice after the namehash for both an indexed and non-indexed value:
ricmoo/ens@ee93577

I'm wondering if in general many of these sorts of things should instead be a Bytes32String instead, since 31 bytes should be enough for most keys? It's non trivial to change now, but would at least result in a different signature.

The indexed looks like it was dropped when the repo was migrated...

[Arachnid]

@ricmoo I'd rather not hard code in a max string length (and assumption of null-termination) just because ABI encoding is inefficient. :)