Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORE-15909: make CurrentGroupParametersLookup public api #1202

Merged
merged 15 commits into from
Aug 25, 2023

Conversation

jennyang-r3
Copy link
Contributor

This PR introduces a new public api for CurrentGroupParametersService that will allow users access to GroupParameters

@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Aug 9, 2023

Jenkins build for PR 1202 build 24

Build Successful:
Jar artifact version produced by this PR: 5.1.0.16-alpha-1692879578824

@jennyang-r3 jennyang-r3 force-pushed the jennyang/CORE-15909-group-parameter-access branch 3 times, most recently from 2992673 to 4cdacd7 Compare August 14, 2023 16:37
@jennyang-r3 jennyang-r3 force-pushed the jennyang/CORE-15909-group-parameter-access branch from 4cdacd7 to 55f685c Compare August 18, 2023 16:13
@jennyang-r3 jennyang-r3 marked this pull request as ready for review August 18, 2023 16:57
@jennyang-r3 jennyang-r3 requested a review from a team as a code owner August 18, 2023 16:57
@jennyang-r3 jennyang-r3 force-pushed the jennyang/CORE-15909-group-parameter-access branch from 974e34c to 81d51f3 Compare August 21, 2023 14:55
lankydan
lankydan previously approved these changes Aug 22, 2023
charlieR3
charlieR3 previously approved these changes Aug 22, 2023
Copy link
Contributor

@charlieR3 charlieR3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

lankydan
lankydan previously approved these changes Aug 22, 2023
@@ -0,0 +1,17 @@
package net.corda.v5.ledger.utxo;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this as a note to myself, so don't worry about it for now.

Should group parameters be available for the consensual ledger as well? If the answer is yes, then GroupParametersLookup should either live in the membership module or the ledger-common module.

There is an argument it should be in the membership module regardless.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blsemo @dimosr @charlieR3 let me know if you have any thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding anything to Consensual Ledger is a product question.
And as I heard, Consensual ledger is not one of the top priorities.
On the other hand, this module does not have anything UTXO-specific.
Also, future ledger modules may need it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's my thinking as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially this should actually go into application along side the MemberLookup interface.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it could fit in application with the MemberLookup interface. The GroupParameters interface is in the membership module already.

Saying that though, the NotaryInfo interface is in membership but NotaryLookup is in ledger-common. I think NotaryInfo is mainly in membership though because GroupParameters depends on it and we don't want membership to depend on ledger.

I would lean towards having it in membership so that it is not ledger specific. We allow adding any custom metadata into to group parameters for example which doesn't necessarily need to be ledger specific.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should consider moving NotaryLookup too (we'd have to deprecate the existing one and copy it to application/membership).

I think feels like the right fit for GroupParametersLookup and possibly NotaryLookup.

Happy to go with the majority on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original logic for NotaryLookup being in ledger-common was that it's tightly linked with the ledger layer so it seems like a better place for it. But not sure that matters much if NotaryInfo is in membership anyway. membership is a bit of a cross layer concept anyway so I'm happy to go with the majority also.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would handle NotaryLookup in a separate ticket.
But I would move GroupParametersLookup to the most future-proof place in this one.
(However, I am not too sure whether application or membership is the best.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets move GroupParametersLookup to v5.application.membership. We can raise a tech debt for NotaryLookup, ideally to be reviewed before 5.1. Then we can decide if they should all move to the membership module or if they all live in v5.application.membership.

@jennyang-r3 jennyang-r3 changed the title CORE-15909: make CurrentGroupParametersService public api CORE-15909: make CurrentGroupParametersLookup public api Aug 23, 2023
@jennyang-r3 jennyang-r3 dismissed stale reviews from lankydan and charlieR3 via 6656356 August 24, 2023 11:04
@jennyang-r3 jennyang-r3 force-pushed the jennyang/CORE-15909-group-parameter-access branch from 5baa9ea to 072476e Compare August 24, 2023 12:19
Copy link
Contributor

@blsemo blsemo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jennyang-r3 jennyang-r3 merged commit 805f96b into release/os/5.1 Aug 25, 2023
1 check passed
@jennyang-r3 jennyang-r3 deleted the jennyang/CORE-15909-group-parameter-access branch August 25, 2023 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants