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

FAB-18067 Discovery support Implicit Collections #2784

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

andrew-coleman
Copy link
Member

Implemented the fix as suggested by Yacov in his Jira comment.

Verified the fix works against the asset-transfer-secured-agreement sample which uses implicit collections.

Signed-off-by: andrew-coleman andrew_coleman@uk.ibm.com

@andrew-coleman andrew-coleman requested a review from a team as a code owner July 26, 2021 12:21
Payload: &peer.CollectionPolicyConfig_SignaturePolicy{
SignaturePolicy: policydsl.SignedByMspMember(mspID),
},
},
Copy link
Contributor

@manish-sethi manish-sethi Jul 27, 2021

Choose a reason for hiding this comment

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

Shouldn't you add here EndorsementPolicy field as well which would carry the same value as for the MemberOrgsPolicy. Look at the following for loop that populates the map ccMetadata.CollectionPolicie with EndorsementPolicy.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that this eventually requires an integration test somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a test to the discovery integration suite

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @andrew-coleman for adding the integration test.

One more point - I am not sure what was the motivation but it appears that we also allow the implicit collection policy to be configured via channel config.

I feel that moving the existing code in a separate function and invoking that function from both the existing code and your code may allow to keep this important logic at one place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Merging this as is for now. Opened a new issue so it can be tracked.

Implemented the fix as suggested by Yacov in his Jira comment.

Verified the fix works against the asset-transfer-secured-agreement sample which uses implicit collections.

Signed-off-by: andrew-coleman <andrew_coleman@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants