Fix get all bonds not returning not active bonds

[ben-kaufman]

Fix #2522
The issue with this is that the proposal was for confiscating a bond which is not active, and the getAllBonds method returned only active bonds, so I changed it to return all bonds instead.

Now the proposals are showing up correctly.

[ManfredKarrer]

Not sure if that is ok, as the ProposalDisplay class uses that as well:
confiscateBondComboBox.setItems(FXCollections.observableArrayList(daoFacade.getAllBonds()));
and there we want only the active bonds.

[ManfredKarrer]

Also used by daoFacade.getBondByLockupTxId which is also used in ProposalDisplay.

[ben-kaufman]

Yes, but it is needed there to display also old items correctly.
For the setItems I'm not sure as you do want to display the link if you choose to display the proposal, but there is no reason to have such an active proposal in the first place.

Anyway, in getBondByLockupTxId we want also non-active bonds so it is possible to see past proposals correctly.

[sqrrm]

The naming of this function is probably not right anyway, better split into getAllBonds() and getActiveBonds(). That would make clearer.

[ben-kaufman]

I agree but there seems to be no need for getActiveBonds() method for now.

[ManfredKarrer]

We need to check if the overall behaviour is correct. Only active and locked bonds should be displayed in the UI for confiscation (if the user has not locked up the bond nothing can be confiscated). Could you check the whole process?

[ben-kaufman]

So the proposal needs to display correctly whenever it shows (even if the bond is not active, like when seeing the vote result history). The case where it should not be displayed is in the active proposals, but there the entire proposal shouldn't be displayed. However I didn't change that behaviour as it seemed out of the scope of this PR and I wasn't sure what should be done with such an "invalid" proposal.

[ManfredKarrer]

I would need more time to checkout the PR and go over the code details but I am too busy atm with other stuff. Feel free to continue as you think it is the best strategy.

[ben-kaufman]

@ManfredKarrer as in some cases inactive bonds may still be confiscatable I just disabled the option to propose confiscating non-active bond and added a "(Inactive Bond)" text to make sure it's easy to notice.

[ManfredKarrer]

as in some cases inactive bonds may still be confiscatable I just disabled the option to propose confiscating non-active bond and added a "(Inactive Bond)" text to make sure it's easy to notice.

Will look into it in the next days. Thanks

[sqrrm]

@ben-kaufman My view is that it doesn't make sense to confiscate inactive bonds. That would mean either confiscating BSQ that's not locked, ie, any BSQ, or unlocked bonds, which are also supposed to work as normal BSQ. That would weaken the trust in the BSQ tokens unnecessarily.

@ManfredKarrer Do you see any use case where we would want to confiscate inactive bonds?

[ben-kaufman]

@sqrrm well it will only be possible to confiscated a bond which was unlocked in the middle of the cycle and wasn't moved.
I can also just filter out those proposals but Im not sure it's good to just hide them like that.

[sqrrm]

@ben-kaufman I'm not sure I follow. Active bonds are those that are either outputs locked with a lock tx or outputs from an unlock tx waiting for the lock time to pass, those outputs I call unlocking.

Any output that is yet to be locked or where the lock time has passed for an unlock tx output I consider inactive, or not bonded. These should be considered normal BSQ.

If a locked output is spent with an unlock tx, this unlocking txoutput is still an active bond and will be considered confiscatable up until lock time has passed. If that tx output is spent before the lock time has passed the BSQ is burnt and no longer considered BSQ.

[ManfredKarrer]

I think @ben-kaufman means that a unlocked bond which has not been transferred to another address could still be confiscated as we know it is the same owner. I discussed that a bit with him. But I think now it is more clean to not consider that case for confiscation. So once a bond has passed the unlock time it cannot be confiscated anymore and should not be displayed in the drop down for confiscatable bonds.

[sqrrm]

@ManfredKarrer Indeed, I think that would be unacceptable. There need to be consistency to the rules and once unlocked it should be as untouchable as any other BSQ. The case where the lock time has passed and the output not yet spend should be covered by having a longer lock time to begin with. The lock time needs to be at least more than one cycle long, most likely 1.5 cycles or perhaps 2 cycles to give enough time for deciding whether a bond should be confiscated.

[ManfredKarrer]

Replaced by #2576