fix: add back rights conservation invariant check #1891
Conversation
|
This is still legible, but it doesn't seem like an improvement to me. I like converting |
There was a problem hiding this comment.
I agree with @katelynsills and @Chris-Hibbert that the new reallocate should have positional rather than named parameters.
Aside from that, I think this delta PR moves in the wrong direction. To make an easy-to-inspect-for invariant more likely to be upheld under maintenance, it restores code that was harder to understand. But I do agree that I should draw attention to the multiple locations in the code that must be coordinated to uphold the invariant. I'll add comments to do so and ask for another round of review.
packages/ERTP/src/issuer.js
Outdated
| reallocate([srcPayment], undefined, currentBalance, newPurseBalance); | ||
| currentBalance = newPurseBalance; |
There was a problem hiding this comment.
Good to see we are thinking along similar lines. I went through a draft like that as well. I rejected it because reallocate is no longer conserving the rights by itself. It requires the following assignment. reallocate is now only checking that the rights would be conserved if followed by that assignment.
The previous idea we both thought about --- having reallocate return the balance --- also has the same problem. Rights are only actually conserved if that assignment happens. But this problem only jumped out at me with the pattern you show here.
packages/ERTP/src/issuer.js
Outdated
| const [payment] = reallocate( | ||
| undefined, | ||
| [amount], | ||
| currentBalance, | ||
| newPurseBalance, | ||
| ); | ||
| currentBalance = newPurseBalance; |
Here's a version of the lexical-balance PR that retains the rights conservation invariant check. I think we might be able to improve the signature of
reallocatefurther.