Add AnonymousParty superclass of Party #215

rnicoll · 2017-02-03T11:01:24Z

Add AnonymousParty superclass of Party in preparation for anonymising parties stored in
contract states.

This is a redesign of the work for #100, although there are further PRs to complete the work. I'm trying to break it into more manageable chunks this time.

mikehearn

This looks good: a much smaller change. Reading it, I wondered if AnonymousParty is the right name. It's the obvious one to choose for sure, but sometimes it seems to imply that the party in question must be anonymous or is perhaps choosing to be anonymous, whereas what we really want to communicate is that the identity doesn't matter. I'm not sure what is better though. UnnamedParty? Sounds like a placeholder where any party could be put. Perhaps AnonymousParty is the best we can do.

mikehearn · 2017-02-06T15:17:17Z

core/src/main/kotlin/net/corda/core/crypto/AnonymousParty.kt

+import java.security.PublicKey
+
+/**
+ * The [Party] class represents an entity on the network, which is typically identified by a legal [name] and public key


The kdoc comment needs to be updated for the move, and Party needs to still have a kdoc comment as well.

mikehearn · 2017-02-06T15:18:18Z

core/src/main/kotlin/net/corda/core/node/services/IdentityService.kt

@@ -23,3 +26,6 @@ interface IdentityService {
    fun partyFromKey(key: CompositeKey): Party?
    fun partyFromName(name: String): Party?
 }
+
+fun IdentityService.deanonymiseParty(party: AnonymousParty) = partyFromKey(party.owningKey)


Why are these extension functions? Is the goal to give these functions better names? If so, let's just add the new names and deprecate the old ones.

They're just nice helper methods to hide the referencing of fields inside the anonymous party. Mostly named differently because I wanted to avoid partyFromParty()

Remember that extension functions aren't really ideal for Java users. You could also just put these in the interface itself, I think. Or add an extension to the Party class:

fun AnonymousParty.deanonymise(service: IdentityService): Party

That would probably make more sense.

mikehearn · 2017-02-06T15:20:42Z

docs/source/key-concepts-core-types.rst


-Identities of parties involved in signing a transaction can be represented simply by a ``CompositeKey``, or by further
+Identities of parties involved in signing a transaction can be represented simply by a ``AnonymousParty``, or by further


This second paragraph is now a little bit duplicative with the prior: you could just lead on with something like "If, on the other hand, a verified identity is required, use the Party class".

mikehearn · 2017-02-06T15:21:15Z

docs/source/release-notes.rst

@@ -3,13 +3,12 @@ Release notes

 Here are brief summaries of what's changed between each snapshot release.

-Milestone 8
+Milestone 9


Eh? The section below this one is Milestone 7. Is the assumption that you'll rebase this onto the M8 release notes before merge?

rnicoll · 2017-02-07T11:26:41Z

That should be ready for re-review @mikehearn

mikehearn

Looks good to me. The only remaining niggle is the extension functions. If we put the methods in the interfaces, then at some point the Kotlin compiler will start emitting Java 8 bytecode that Java understands as default methods in interfaces too, I think. At any rate, for core APIs let's try and avoid extension functions which show up as static utility methods to Java users.

rick-r3 · 2017-02-07T12:10:03Z

core/src/main/kotlin/net/corda/core/crypto/AnonymousParty.kt

+    constructor(owningKey: PublicKey) : this(owningKey.composite)
+
+    /** Anonymised parties do not include any detail apart from owning key, so equality is dependent solely on the key */
+    override fun equals(other: Any?): Boolean = other is Party && this.owningKey == other.owningKey


Is is Party here correct? Surely should be AnonymousParty ? With the current code no two AnonymousParty would ever be equal. e.g. Can't use them in a HashMap.

rick-r3 · 2017-02-07T12:11:39Z

core/src/main/kotlin/net/corda/core/node/services/IdentityService.kt

@@ -23,3 +26,6 @@ interface IdentityService {
    fun partyFromKey(key: CompositeKey): Party?
    fun partyFromName(name: String): Party?
 }
+
+fun IdentityService.deanonymiseParty(party: AnonymousParty) = partyFromKey(party.owningKey)


Should this check if the party is a Party and do no work if so?

rnicoll · 2017-02-07T12:45:19Z

@rick-r3 I've changed the equality method (good catch), and had it check for the type being 'Party' and return as-is in that case.

I've also renamed the function to better fit the naming scheme used, and avoided an extension method based on Mike's feedback.

mikehearn · 2017-02-07T15:51:53Z

Probably we should have a few unit tests that would have caught what Rick just spotted! Can you add some?

rnicoll · 2017-02-07T17:14:39Z

Tests added

rnicoll · 2017-02-08T12:02:20Z

Rebased to keep up to date with master. Could I get a review please @rick-r3 so I can get this merged before anything more changes? :)

rick-r3

See minor comment. Is everyone (mostly) happy this is the direction?

rick-r3 · 2017-02-08T14:50:14Z

core/src/main/kotlin/net/corda/core/crypto/Party.kt

-
-    /** Anonymised parties do not include any detail apart from owning key, so equality is dependent solely on the key */
-    override fun equals(other: Any?): Boolean = other is Party && this.owningKey == other.owningKey
-    override fun hashCode(): Int = owningKey.hashCode()
    override fun toString() = name


Minor. We should include the pub key in toString as well, or incorporate super.toString() in some way, since that is what AnonymousParty does. Or does it break some tests / expectations somewhere? Thinking maybe the JSON marshalling for demos assumes what toString does... in which case maybe make it a TODO. Just thinking debugging or logging will seem weird if one shows a pub key and the other shows the text name. Makes hard to understand they represent same party.

I think I've removed assumptions about what toString() produces, earlier. Have added a commit to make the results more predictable.

Add AnonymousParty superclass of Party in preparation for anonymising parties stored in contract states. Signed-off-by: Ross Nicoll <ross.nicoll@r3.com>

rnicoll added the enhancement label Feb 3, 2017

rnicoll added this to the M9 milestone Feb 3, 2017

rnicoll requested review from mikehearn, exFalso and rick-r3 February 3, 2017 11:01

mikehearn suggested changes Feb 6, 2017

View reviewed changes

rnicoll force-pushed the rnicoll-state-party-2 branch 2 times, most recently from f852531 to c165420 Compare February 6, 2017 18:35

mikehearn approved these changes Feb 7, 2017

View reviewed changes

rick-r3 requested changes Feb 7, 2017

View reviewed changes

rnicoll force-pushed the rnicoll-state-party-2 branch from c165420 to b2aebde Compare February 7, 2017 12:35

rnicoll force-pushed the rnicoll-state-party-2 branch from 1f69117 to 36c8339 Compare February 8, 2017 11:30

rick-r3 approved these changes Feb 8, 2017

View reviewed changes

Add AnonymousParty superclass of Party

9785cd9

Add AnonymousParty superclass of Party in preparation for anonymising parties stored in contract states. Signed-off-by: Ross Nicoll <ross.nicoll@r3.com>

rnicoll force-pushed the rnicoll-state-party-2 branch from eb90d78 to 9785cd9 Compare February 8, 2017 17:23

rnicoll merged commit 47d2606 into master Feb 9, 2017

rnicoll deleted the rnicoll-state-party-2 branch February 9, 2017 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AnonymousParty superclass of Party #215

Add AnonymousParty superclass of Party #215

rnicoll commented Feb 3, 2017

mikehearn left a comment

mikehearn Feb 6, 2017

mikehearn Feb 6, 2017

rnicoll Feb 6, 2017

mikehearn Feb 7, 2017

mikehearn Feb 6, 2017

mikehearn Feb 6, 2017

rnicoll commented Feb 7, 2017

mikehearn left a comment

rick-r3 Feb 7, 2017

rick-r3 Feb 7, 2017

rnicoll commented Feb 7, 2017

mikehearn commented Feb 7, 2017

rnicoll commented Feb 7, 2017

rnicoll commented Feb 8, 2017

rick-r3 left a comment

rick-r3 Feb 8, 2017

rnicoll Feb 8, 2017


		Identities of parties involved in signing a transaction can be represented simply by a ``CompositeKey``, or by further
		Identities of parties involved in signing a transaction can be represented simply by a ``AnonymousParty``, or by further

Add AnonymousParty superclass of Party #215

Add AnonymousParty superclass of Party #215

Conversation

rnicoll commented Feb 3, 2017

mikehearn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnicoll commented Feb 7, 2017

mikehearn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnicoll commented Feb 7, 2017

mikehearn commented Feb 7, 2017

rnicoll commented Feb 7, 2017

rnicoll commented Feb 8, 2017

rick-r3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment