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.

Sign up for GitHub

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

Jump to bottom

feat: Update specification text for address book service #346

Merged
merged 1 commit into from
Jun 12, 2024
Merged

feat: Update specification text for address book service #346

merged 1 commit into from
Jun 12, 2024

Conversation

jsync-swirlds
Copy link
Member

@jsync-swirlds jsync-swirlds commented Apr 27, 2024 •
edited
Loading

  • Updated specification/API comments

Rendered documentation

A rendered version of the documentation in these files is temporarily available on another branch to assist with visualizing what these changes accomplish.

@jsync-swirlds jsync-swirlds changed the title Update specification text for address book service feat: Update specification text for address book service Apr 27, 2024
@jsync-swirlds jsync-swirlds marked this pull request as ready for review April 27, 2024 00:30
@jsync-swirlds jsync-swirlds requested review from a team as code owners April 27, 2024 00:30
@jsync-swirlds jsync-swirlds self-assigned this Apr 27, 2024
iwsimon
iwsimon reviewed May 6, 2024
View reviewed changes
services/node_create.proto Outdated Show resolved Hide resolved
iwsimon
iwsimon reviewed May 6, 2024
View reviewed changes
services/node_create.proto Outdated Show resolved Hide resolved
iwsimon
iwsimon reviewed May 6, 2024
View reviewed changes
services/node_create.proto Outdated Show resolved Hide resolved
iwsimon
iwsimon reviewed May 6, 2024
View reviewed changes
services/node_create.proto Outdated Show resolved Hide resolved
iwsimon
iwsimon reviewed May 6, 2024
View reviewed changes
services/node_create.proto Outdated Show resolved Hide resolved
@jsync-swirlds jsync-swirlds force-pushed the 9796-DAB-Update-Specification branch 6 times, most recently from a41924d to a596ed0 Compare May 7, 2024 19:07
@jsync-swirlds
Copy link
Member Author

(small comment) Can we change the name of node_create to node_add? I understand the CRUD structure, but node_add is more descriptive of the task.

  1. @iwsimon would be the best individual to do that in the PR to add all these files back into the protobuf repo (everything related to DAB was removed from both protobuf and services codebase on Wednesday).
  2. The HIP would need to be rewritten to change all the create to add wording.
  3. That would be inconsistent with every other service use of create for adding or creating an entity
  4. It is creating an entity in state, so create seems entirely reasonable to my mind.

@jsync-swirlds jsync-swirlds force-pushed the 9796-DAB-Update-Specification branch from a1cff62 to 597d76b Compare June 3, 2024 17:05
@jsync-swirlds
Copy link
Member Author

Updated following the removal and re-add of all address book changes.
That bounce required squashing these commits to make rebase sane.

@jsync-swirlds
Copy link
Member Author

Please remove the status property and NodeStatus. The design for this functionality is incomplete and the usage of status and NodeStatus is not a given.

@netopyr This is done, along with matching the most recent discussions and updates. Please re-review.

@jsync-swirlds jsync-swirlds added this to the v0.52 milestone Jun 4, 2024
@jsync-swirlds jsync-swirlds force-pushed the 9796-DAB-Update-Specification branch from 597d76b to 580c35b Compare June 5, 2024 16:15
@jsync-swirlds jsync-swirlds added documentation Improvements or additions to documentation and removed documentation Improvements or additions to documentation labels Jun 5, 2024
@jsync-swirlds
Update specification text for address book service
22d6126 
* Updated specification/API comments
* Consolidated review changes
   * Adjusted wording for consistency and clarity
   * Adjusted formatting for consistency
   * Updated query.proto with package changes
   * Corrected gossip certificate encoding
   * Adjusted gossip endpoint to state that networks may or
     may not require those endpoints to have or not have
     a DNS entry.
      * Further clarified that DNS vs. Address is network-specific.
      * Matched updated wording based on recent discussions
   * Removed references to status in state Node message.
   * Removed status enum as requested.
   * Added deleted flag to match recent updates.

Signed-off-by: Joseph Sinclair <[email protected]>
@jsync-swirlds jsync-swirlds force-pushed the 9796-DAB-Update-Specification branch from 580c35b to 22d6126 Compare June 5, 2024 23:05
This was referenced Jun 10, 2024
Closed
Open
Closed
Open
Closed
Closed
netopyr
netopyr reviewed Jun 11, 2024
View reviewed changes
Copy link
Contributor

@netopyr netopyr left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @jsync-swirlds

povolev15
povolev15 approved these changes Jun 12, 2024
View reviewed changes
Copy link
Contributor

@povolev15 povolev15 left a comment

Choose a reason for hiding this comment

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

LGTM

@jsync-swirlds jsync-swirlds merged commit 404d574 into main Jun 12, 2024
1 check passed
@jsync-swirlds jsync-swirlds deleted the 9796-DAB-Update-Specification branch June 12, 2024 15:18
netopyr added a commit that referenced this pull request Jul 18, 2024
@netopyr
Merge branch 'main' into tmp-hip796-protos-test
146d477 
* main: (21 commits)
  feat: 13135 Added PlatformState protobuf representation (#349)
  feat: Change AddressBookService package to proto (#386)
  feat: add admin_key to node_create.proto, node_update.proto and node.proto (#380)
  HIP 904 proto updates for token reject (#367)
  chore: Updated codeowners to include devops-ci (#384)
  feat: remove node_get_info.proto (#378)
  fixed INVALID_GOSSIP_CA_CERTIFICATE (#375)
  fix: Fixing single-letter mismatch to expectations (#373)
  feat: Update specification text for address book service (#346)
  feat: Add more response codes for Dynamic Address Book Phase 2 (#370)
  feat: maxAutoAssociations updates for contract transactions (#359)
  Added more response codes for Dynamic Address Book Phase 2 (#364)
  feat: Add back Dynamic Address Book Phase 2 protobufs change (#361)
  feat: Back out protobuf-changes to dynamic address book for release 0.51 (#358)
  feat: added NodeGetInfoResponse to response.proto (#355)
  feat: HIP-904 maxAutoAssociations updates (#348)
  chore: added consensus data protobuf (#350)
  Added NodeGetInfo (#347)
  feat: Dynamic Address Book Phase 2 protobufs changes (#344)
  feat: define event protobufs (#338)
  ...

# Conflicts:
#	services/basic_types.proto
#	services/schedulable_transaction_body.proto
#	services/token_service.proto
#	services/transaction_body.proto
#	services/transaction_receipt.proto
netopyr added a commit that referenced this pull request Jul 18, 2024
@netopyr
Merge branch 'main' into tmp-hip796-protos
1e2cea0 
* main: (21 commits)
  feat: 13135 Added PlatformState protobuf representation (#349)
  feat: Change AddressBookService package to proto (#386)
  feat: add admin_key to node_create.proto, node_update.proto and node.proto (#380)
  HIP 904 proto updates for token reject (#367)
  chore: Updated codeowners to include devops-ci (#384)
  feat: remove node_get_info.proto (#378)
  fixed INVALID_GOSSIP_CA_CERTIFICATE (#375)
  fix: Fixing single-letter mismatch to expectations (#373)
  feat: Update specification text for address book service (#346)
  feat: Add more response codes for Dynamic Address Book Phase 2 (#370)
  feat: maxAutoAssociations updates for contract transactions (#359)
  Added more response codes for Dynamic Address Book Phase 2 (#364)
  feat: Add back Dynamic Address Book Phase 2 protobufs change (#361)
  feat: Back out protobuf-changes to dynamic address book for release 0.51 (#358)
  feat: added NodeGetInfoResponse to response.proto (#355)
  feat: HIP-904 maxAutoAssociations updates (#348)
  chore: added consensus data protobuf (#350)
  Added NodeGetInfo (#347)
  feat: Dynamic Address Book Phase 2 protobufs changes (#344)
  feat: define event protobufs (#338)
  ...
vtronkov pushed a commit that referenced this pull request Jul 23, 2024
@jsync-swirlds @vtronkov
feat: Update specification text for address book service (#346)
edfaadd 
Update specification text for address book service
* Updated specification/API comments
* Consolidated review changes
   * Adjusted wording for consistency and clarity
   * Adjusted formatting for consistency
   * Updated query.proto with package changes
   * Corrected gossip certificate encoding
   * Adjusted gossip endpoint to state that networks may or
     may not require those endpoints to have or not have
     a DNS entry.
      * Further clarified that DNS vs. Address is network-specific.
      * Matched updated wording based on recent discussions
   * Removed references to status in state Node message.
   * Removed status enum as requested.
   * Added deleted flag to match recent updates.

Signed-off-by: Joseph Sinclair <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeffreyDallas JeffreyDallas JeffreyDallas approved these changes

@povolev15 povolev15 povolev15 approved these changes

@netopyr netopyr netopyr approved these changes

@jasperpotts jasperpotts Awaiting requested review from jasperpotts

@steven-sheehy steven-sheehy Awaiting requested review from steven-sheehy

@tomzhenghedera tomzhenghedera Awaiting requested review from tomzhenghedera tomzhenghedera was automatically assigned from hashgraph/release-engineering

@edward-swirldslabs edward-swirldslabs Awaiting requested review from edward-swirldslabs

@nathanklick nathanklick Awaiting requested review from nathanklick

@iwsimon iwsimon Awaiting requested review from iwsimon

@Mark-Swirlds Mark-Swirlds Awaiting requested review from Mark-Swirlds

Assignees

@jsync-swirlds jsync-swirlds

Labels
None yet
Projects
None yet
Milestone
v0.52
Development

Successfully merging this pull request may close these issues.
7 participants
@jsync-swirlds @netopyr @povolev15 @JeffreyDallas @iwsimon @edward-swirldslabs @Mark-Swirlds