<p></p>
<p><a class="user-mention" data-hovercard-type="user" data-hovercard-url="/users/freimair/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/freimair">@freimair</a> followed up above post with a Keybase message and we had a brief exchange there. I'll summarize how I see this situation and what my concerns are:</p>
<ul>
<li>P2P datastore code has a fair amount of technical debt, which makes making changes to it without compromises slow;</li>
<li>we want to introduce changes quickly, which encourages us to make compromises;</li>
<li>compromising changes tend to degrade reliability and are more likely to introduce mistakes;</li>
<li>compromising changes require more resources to maintain: freimair points out that there are plans to replace large portions of this code entirely, which means that if all goes well those portions will not require maintenance;</li>
<li>compromising changes require more resources to review.</li>
</ul>
<p>My concerns:</p>
<ul>
<li>I've spent a long time on the PR (~2.5 days, I realise this says as much about my reviewing ability as it does about the PR), but I'm still not confident that all edge cases are covered; I don't feel comfortable seeing this merged at this time;</li>
<li>I think that reliability, maintainability and reviewability are preferable to speed, especially in sensitive code, though speed has its importance too; it's a balancing act;</li>
<li>introducing changes that are difficult to maintain, because if all goes well they won't have to be maintained, is planning for the best outcome; I don't think that's prudent;</li>
<li>I think it's good to address this human element: PRs that make hasteful changes, or side-step technical debt, often require the reviewer to address that complexity anyway, so that he can make sure that the changes are correct, which can be frustrating for the reviewer (who usually always has his plate full as is).</li>
</ul>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/4233#issuecomment-645293913">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJFFTNUUBXRPZIF7YDABJW3RXCLYVANCNFSM4MY33J4A">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AJFFTNSRODX5W35PN6YPCK3RXCLYVA5CNFSM4MY33J4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEZ3GOWI.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/4233#issuecomment-645293913",
"url": "https://github.com/bisq-network/bisq/pull/4233#issuecomment-645293913",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>