feat!: add a claimsRegistrar based on attestations

[Chris-Hibbert]

Agents who can deposit an attestation payment will get the ability to vote that amount.
refactor common registrat code to a library
tests that BinaryBallotCounter can count these votes.

[dckc]

after poring over the code, I packaged up a bunch of suggestions in #3932

What's here is a good step forward with or without those suggestions.

[dckc]

"the lean"? What does that definite description refer to?

[Chris-Hibbert]

I'll assume "the lean" is auto-corrupted from "the lien".

There's an underlying lien for the tokens represented in the go-code. When the holder extends the duration (locks up for an extended period) or adds to the deposited tokens, the attestation contract makes sure to use the same identity, so that attestation payments are re-usable without confusing vote counters, even though the value locked or duration of the lock can change over time.

[dckc]

This is sort of a copy-editing / tech writing comment. Whenever I see a definite description that doesn't have a clear referent, my mind flags it like a compiler error. Usually making the referent clear is straightforward, but sometimes the ill-formed definite description is a sign of a larger problem with the text or even the design, as if we were referring to "the solution to the equation" when there may in fact be none or more than one.

My first reaction to this comment was to wonder what it is about; i.e. what it is documenting. It would make sense to make it a /** JSDoc comment on start or to use JSDoc's @file. But we've had that discussion before, so I let that go.

Then I bumped over "shareHolders is ..." -- a plural subject with a singular verb. But OK, I can read it as "The shareHolders contract is..." or some such.

Then "relies on an attestation contract" reminds me of a long-standing issue I've had with the platform: what is a contract? I know what a bundle, an installation, and an instance are. I read it as an instance in this case. The instance isn't in the terms, but a corresponding brand is, and it's clear enough how they relate after reading attestation/expiring/expiringNFT.js and such.

"to ensure that only valid holders of shares have the ability to vote" somewhat raises the question of what a valid holder of a share is; on the other hand, it defines it in terms of capabilities. So very well.

"each votable share has a persistent handle" sounds at first like if I have 1000 shares then I get 1000 handles. Perhaps "... for every votable share, there is a persistent handle..." would be better, but I figured it out by reading attestation.js etc. and forgot to comment on that part.

But then we get to "extending the duration of the lien" and nothing in this comment introduced a lien into the discussion.

"the attestations" follows "an attestation contract" soon enough to allude to it, though on review, it's not clear enough to serve as a specification.

I wonder if it would be cost-effective for this comment to start with attestationBrand and tell the rest of the story from there.

In the end, my code review bar is: am I OK maintaining this code? If I'm called on to work on this file, there's a good chance I'll refine that comment. But I think I understand it well enough to endorse the code as-is.

[dckc]

Making up all these names for things we only use once only makes the code harder to read, to my mind. I suggest:

Suggested change

	/**
	* @typedef {Object} CommitteeElectoratePublicMixin
	* @property {() => string} getName
	*/
	/**
	* @typedef {Object} ClaimsElectorateMixin
	* @property {() => ERef<Invitation>} makeVoterInvitation
	*/
	/**
	* @typedef { ElectoratePublic & ClaimsElectorateMixin } ClaimsElectoratePublic
	* @typedef { ElectoratePublic & CommitteeElectoratePublicMixin } CommitteeElectoratePublic
	* @typedef { ClaimsElectoratePublic | CommitteeElectoratePublic } AnyElectoratePublic
	*/
	/**
	* @typedef { ElectoratePublic & {makeVoterInvitation: () => ERef<Invitation>} } ClaimsElectoratePublic
	* @typedef { ElectoratePublic & {getName: () => string} } CommitteeElectoratePublic
	*/

[Chris-Hibbert]

That is simpler for this case. It feels like if there were more than one added method, we'd need to break it out for readability anyway. Do you think the in-line approach would apply more generally?

[dckc]

Do you think the in-line approach would apply more generally?

I do, though I gather it goes against an established norm to some extent.

[Chris-Hibbert]

I incorporated the suggestions from #3932, though not always the way you wrote them.

[dckc]

I incorporated the suggestions from #3932, though not always the way you wrote them.

Cool. That's the spirit in which they were offered.