Change address constructors to explicit

[abitmore]

This PR is mainly for making clear how public keys are being ordered in the system, aka explicitly implement

bool operator < ( const public_key_type&, const public_key_type&);

and make sure it's backward-compatible (convert the public keys to addresses and compare the addresses).

More info is here: cryptonomex/graphene#630 (comment) .

The == operators are mainly used in balance_evaluator.cpp, actually I'm not sure whether it's good to have them (does it make sense to compare a pts_address with an address, or compare a public key to an address?), please advise.

bitshares-core/libraries/chain/balance_evaluator.cpp

Lines 33 to 38 in 76ab28e

	GRAPHENE_ASSERT(
	op.balance_owner_key == balance->owner ||
	pts_address(op.balance_owner_key, false, 56) == balance->owner ||
	pts_address(op.balance_owner_key, true, 56) == balance->owner ||
	pts_address(op.balance_owner_key, false, 0) == balance->owner ||
	pts_address(op.balance_owner_key, true, 0) == balance->owner,

[jmjatlanta]

The equality operator here boils down to a memcmp of the 2 ripemd160 hashes.

https://github.com/bitshares/bitshares-fc/blob/d679377312d715840408d2990f824d2c6e729aea/src/crypto/ripemd160.cpp#L98-L100

Are you attempting to replace the GRAPHENE_ASSERT with something else? What would you suggest they be replaced with?

Perhaps I am misunderstanding why you are questioning whether it is good to have the equality operators.

[abitmore]

My question was: does it make sense to compare a pts_address with an address, or compare a public key to an address?

[pmconrad]

As mentioned on Telegram, I'm undecided if we should merge this in the upcoming release.

I generally think explicit constructors are better, because that avoids surprising automatic conversion.

OTOH making existing constructors explicit can have surprising side effects, so the change is a bit dangerous.

[pmconrad]

Surprising side-effects could be avoided by changing all one-argument constructors to explicit.
SonarQube lists 61 such issues in the core code, not counting those in fc nor test suites.

[abitmore]

Makes sense. I've removed it from this milestone.

[ryanRfox]

Is there a desire to revisit this PR for the 201810 Feature Release?

[pmconrad]

Like I said, we should make all one-arg constructors explicit in one go. But I think we have more important stuff and little time for the next release, so I'd drop it from this one.

[abitmore]

Moving to next milestone.

[ryanRfox]

@pmconrad Sorry, my meeting notes are lacking, so request you comment here with proper instructions for this PR. This is what I captured: "recommendation for now until we decide how to do all explicit constructors"

[pmconrad]

See my comment above - merging this now could have unforeseeable side effects. Better to switch all one-arg constructors to explicit at once.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

90.0% Coverage
0.0% Duplication