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

Introduce Anchor peer API and other refractoring #120

Merged
merged 3 commits into from
Jan 11, 2024

Conversation

nidhi-singh02
Copy link
Contributor

No description provided.

@nidhi-singh02 nidhi-singh02 requested a review from a team as a code owner January 2, 2024 09:42
@nidhi-singh02 nidhi-singh02 marked this pull request as draft January 2, 2024 11:28
@nidhi-singh02 nidhi-singh02 force-pushed the anchorPeer branch 4 times, most recently from f827021 to fa17182 Compare January 3, 2024 17:31
@nidhi-singh02 nidhi-singh02 marked this pull request as ready for review January 4, 2024 04:52
Copy link
Member

@arsulegai arsulegai left a comment

Choose a reason for hiding this comment

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

Looks good, there's possibility of removing duplicate code.

@PostMapping(value = "/channel/{channelName}/add_anchor_peer")
public ResponseEntity<ClientResponseModel> addAnchorPeersToChannel(
@PathVariable @Validated String channelName,
@RequestBody @Validated ChannelUpdateParamsDTO channelUpdateParamsDTO) {
Copy link
Member

Choose a reason for hiding this comment

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

[nitpick/question] Will it make it simple to have one channel update API with different query parameter to do different activities? The other inputs to the API remain the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How I visual this - two operations that requires channel update are performed at different time, like one is done to add new organization to the channel and the other - to add anchor peers to a channel is done once the peer has joined the channel. It is good to keep them as separate since these are entirely different operation and most importantly, impacting the different sections in the config block.
If we see a need to merge them in the future - by introducing more scenarios that require config block update, we could do it.

MessageOrBuilder message = configUpdate;
String channelConfigString = JsonFormat.printer().print(message);
ConfigUpdate configUpdate = createConfigUpdate(channelName, organizationDetails);
String channelConfigString = JsonFormat.printer().print(configUpdate);
Copy link
Member

Choose a reason for hiding this comment

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

[question] The method getDeserializedConfig appears to be doing this task with the exception handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The existing logic was to print it as string and use it further, kept the logic as in. The getDeserializedConfig method returns the Base64 encoded characters.

try {
Channel selectedChannel = network.getChannel();
ConfigUpdate configUpdate = createConfigUpdate(channelName, channelUpdateParamsDTO);
String channelConfigString = JsonFormat.printer().print(configUpdate);
Copy link
Member

Choose a reason for hiding this comment

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

[question] The method getDeserializedConfig appears to be doing this task with the exception handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The getDeserializedConfig method returns the Base64 encoded characters. Here we are interested in converting a protobuf message to JSON format, that's why didn't reuse the method.

@nidhi-singh02 nidhi-singh02 merged commit 7729b7d into hyperledger-labs:main Jan 11, 2024
4 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.

3 participants