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

Add multiparty status API #1509

Merged
merged 10 commits into from
May 28, 2024
Merged

Conversation

chrisbygrave
Copy link
Contributor

No description provided.

Signed-off-by: Chris Bygrave <[email protected]>
@chrisbygrave chrisbygrave marked this pull request as ready for review May 9, 2024 13:36
@chrisbygrave chrisbygrave requested a review from a team as a code owner May 9, 2024 13:36
Signed-off-by: Chris Bygrave <[email protected]>
- derived from catchup

Signed-off-by: Chris Bygrave <[email protected]>
Signed-off-by: Chris Bygrave <[email protected]>
}

// Ensure the given registration message correlates with the expected type
func (or *orchestrator) checkRegistrationType(ctx context.Context, msg *core.MessageInOut, expectedType core.IdentityType) (match bool, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this check in response to a specific error scenario we've seen? I'm trying to think why there would be a pending registration message, but the contents would be inconsistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it was purely theoretical - it felt clumsy before I added this that we would find a registration message and just assume it was of the correct type. It's technically possible that a pending node registration message could be there without the org being registered, and the API would then return the wrong response.

I admit it could be overkill...

- also added a missing test around retrieving active listener status

Signed-off-by: Chris Bygrave <[email protected]>
Copy link
Contributor

@EnriqueL8 EnriqueL8 left a comment

Choose a reason for hiding this comment

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

LGTM and merging! Thanks @chrisbygrave

@EnriqueL8 EnriqueL8 merged commit 3445132 into hyperledger:main May 28, 2024
14 checks passed
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.

None yet

3 participants