UINT256_MAX in Domain Proofs

[kronosapiens]

The issue

Taking DLP actions (see #553) requires submitting a "domain proof" of the form (permissionDomainId, childSkillIndex, domainId). These three inputs allow us to verify, in constant time, that both a) the user has the permission needed to take the action, and b) the domain of action is in the inheritance path of the domain of permission. There was one hiccup -- if permissionDomainId and domainId were the same (i.e. a user was taking an action in the same domain where they had the permission, a common occurrence), then the semantics of childSkillIndex became murky -- since children are 0-indexed, childSkillIndex = 0 meant the "first child" -- there was no value which represented "the domain itself".

Our initial solution was to say, "alright, when permissionDomainId == domainId, we just ignore childSkillIndex" -- the user can put in whatever value they want, we just ignore it. That worked pretty well.

However, that degree of ambiguity, where (permissionDomainId, childSkillIndex, domainId) is not strictly unique, came back to bite us in the butt when implementing disputes.

Why? A confluence of two factors. First, in order to make disputes as general as possible, all actions are represented as bytes arrays, to be executed by the disputes extension. Second, in order to ensure that only appropriate reputation is voting on disputes, we must be able to infer, from the action, what domain it acts in. How to do this? By parsing the bytes array, extracting the permissionDomainId and childSkillIndex (which, by fortunate convention, are always the first two arguments), and using those to infer domainId.

Herein lies the problem: it is not always possible to exactly infer domainId from the other two arguments, as a childSkillIndex of 0 can be interpreted to mean EITHER permissionDomainId itself OR the first child. This creates a problem for disputes since it means that an action in a domain could potentially be voted on by the first child domain, a clear violation of the semantics of DLP.

The answer

Fortunately, we have a solution, and it is relatively simple: adopt a new convention, where UINT256_MAX (the largest possible number) is passed as childSkillIndex whenever permissionDomainId == domainId. In a sense, it becomes a "pseudo index" for a non-existent value.

While a slight hack, this approach is justifiable in that there is no way for any domain to have UINT256_MAX child domains (for reasons of gas limits and also of time), so there will never be a case where UINT256_MAX actually indexes a child domain, making it's usefulness as a special index valid.

The consequences

Fortunately, since childSkillIndex must almost always be supplied by users, this change will not* break any existing contracts. It may, however, require client updates to ensure that the correct value is being passed. These updates can be as simple as a line to the effect of:

const childSkillIndex = (permissionDomainId == domainId) ? UINT256_MAX : getIndex(permissionDomainId, domainId);

*Unfortunately, OneTxPayment will need to be updated, since that contract does have childSkillIndex hardcoded for the root domain.

[area]

This is a good change - nothing substantive comment wise from me.

Noting for completeness on GH that we can't merge yet. Because of the changes required to OneTxPayment, a colony using the app that upgraded to a version with this change will need to upgrade OneTxPayment. This will require changes to the dapp, but ideally it'd be changes that supported #714 in general and made extension upgrades straightforward in the future.

[area]

Noting for completeness on GH that we can't merge yet. Because of the changes required to OneTxPayment, a colony using the app that upgraded to a version with this change will need to upgrade OneTxPayment. This will require changes to the dapp, but ideally it'd be changes that supported #714 in general and made extension upgrades straightforward in the future.

Now that unsupported upgrades are not displayed in the app, we have merged this, but it is still true. If #714 is done by the time this is deployed, the app will have to support that, otherwise the app will have to, as part of the upgrade process, deploy a new OneTxPayment extension (which is a flow that will have to be able to be recovered from - something I know has given us grief in the past).