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

6 - Ledger API changes for UM/UP extensions for Hub [DPP-1211] #14937

Merged
merged 23 commits into from
Sep 26, 2022
Merged

6 - Ledger API changes for UM/UP extensions for Hub [DPP-1211] #14937

merged 23 commits into from
Sep 26, 2022

Conversation

pbatko-da
Copy link
Contributor

@pbatko-da pbatko-da commented Sep 5, 2022 •
edited
Loading

changelog_begin

  • [Ledger API Specification]:
    • Add 'is_deactivated' attribute to participant users.
    • Introduce participant server's local metadata for parties and participant users consisting of:
      • a resource version for optional concurrent change control,
      • modifiable key-value based annotations.
    • Add experimental update RPCs for parties and participant users:
      • participant user's modifiable fields are: 'primary_party', 'is_deactivated', 'metadata.annotations',
      • party's modifiable fields are 'local_metadata.annotations'.
    • Discourage use of 'party_details.display_name' in favor of using party's metadata annotations.
      changelog_end

Other notes:

  • no-op updates are allowed,
  • update paths are resource relative,
  • as annotation deletion is by providing an key with the empty string value,
  • as a corollary, only non-empty strings are valid annotation values.

@pbatko-da pbatko-da self-assigned this Sep 5, 2022
@pbatko-da pbatko-da force-pushed the pbatko/pbatko-um-6-api-service branch 12 times, most recently from 3772677 to c21bb71 Compare September 13, 2022 14:47
@pbatko-da pbatko-da force-pushed the pbatko/pbatko-um-6-api-service branch 2 times, most recently from fded063 to 14c3503 Compare September 13, 2022 19:40
@pbatko-da pbatko-da changed the title 06 - Service api layer [DPP-1211][WIP] 06 - Ledger API changes for UM/UP extensions for Hub [DPP-1211] Sep 14, 2022
@pbatko-da pbatko-da changed the title 06 - Ledger API changes for UM/UP extensions for Hub [DPP-1211] 6 - Ledger API changes for UM/UP extensions for Hub [DPP-1211] Sep 14, 2022
@pbatko-da pbatko-da marked this pull request as ready for review September 14, 2022 13:04
carrielaben-da
carrielaben-da reviewed Sep 14, 2022
View reviewed changes
Copy link
Contributor

@carrielaben-da carrielaben-da left a comment

Choose a reason for hiding this comment

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

Mostly grammar/punctuation fixes.

docs/resources/generated-error-pages/error-codes-inventory.rst.inc

**Category**: ContentionOnSharedResources

**Conveyance**: This error is logged with log-level INFO on the server side. This error is exposed on the API with grpc-status ABORTED including a detailed error message
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**Conveyance**: This error is logged with log-level INFO on the server side. This error is exposed on the API with grpc-status ABORTED including a detailed error message
**Conveyance**: This error is logged with log-level INFO on the server side and exposed on the API with grpc-status ABORTED including a detailed error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a separate PR to improve the conveyance wording, because we generate them from the same templates for all the error codes.
PR: #15043

(I'll be also marking all conveyance related comments in this PR as resolved.)

docs/resources/generated-error-pages/error-codes-inventory.rst.inc Outdated Show resolved Hide resolved
docs/resources/generated-error-pages/error-codes-inventory.rst.inc Outdated Show resolved Hide resolved
docs/resources/generated-error-pages/error-codes-inventory.rst.inc Show resolved Hide resolved
docs/resources/generated-error-pages/error-codes-inventory.rst.inc Outdated Show resolved Hide resolved
docs/resources/generated-error-pages/error-codes-inventory.rst.inc Show resolved Hide resolved
docs/resources/generated-error-pages/error-codes-inventory.rst.inc Outdated
MAX_USER_ANNOTATIONS_SIZE_EXCEEDED
---------------------------------------------------------------------------------------------------------------------------------------

**Explanation**: A user can have only a limited amount of annotations. There was an attempt to create or update a user with too many annotations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**Explanation**: A user can have only a limited amount of annotations. There was an attempt to create or update a user with too many annotations.
**Explanation**: A user can have only a limited number of annotations. There was an attempt to create or update a user with too many annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to MAX_PARTY_DETAILS_ANNOTATIONS_SIZE_EXCEEDED I'm rewording this to state:

A user can have at most 256kb worth of annotations in total measured in number of bytes in UTF-8 encoding.
There was an attempt to create or update a user such that this limit would have been exceeded.

docs/resources/generated-error-pages/error-codes-inventory.rst.inc Show resolved Hide resolved
docs/resources/generated-error-pages/error-codes-inventory.rst.inc Outdated Show resolved Hide resolved
docs/resources/generated-error-pages/error-codes-inventory.rst.inc Outdated Show resolved Hide resolved
@pbatko-da pbatko-da force-pushed the pbatko/pbatko-um-6-api-service branch from 2a87414 to dd92638 Compare September 22, 2022 10:28
mziolekda
mziolekda approved these changes Sep 22, 2022
View reviewed changes
Copy link
Contributor

@mziolekda mziolekda left a comment

Choose a reason for hiding this comment

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

I have left a couple of final cosmetic comments. Otherwise it looks good.
Well done!

@pbatko-da pbatko-da enabled auto-merge (squash) September 26, 2022 07:36
@pbatko-da pbatko-da merged commit ca81ade into main Sep 26, 2022
@pbatko-da pbatko-da deleted the pbatko/pbatko-um-6-api-service branch September 26, 2022 12:56
@pbatko-da pbatko-da mentioned this pull request Sep 28, 2022
Open
pbatko-da
pbatko-da commented Sep 28, 2022
View reviewed changes
ledger/ledger-api-test-tool-on-canton/BUILD.bazel
@@ -79,6 +79,14 @@ conformance_test_tool_args = [
"TLSAtLeastOnePointTwoIT",
"CommandDeduplicationPeriodValidationIT:OffsetPruned", # requires pruning not available in canton community
"DeeplyNestedValueIT", # FIXME: Too deeply nested values flake with a time out (half of the time)
# TODO um-for-hub: Enable these tests once Canton runs updated user and party management
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fyi @danilofaria

pbatko-da
pbatko-da commented Sep 28, 2022
View reviewed changes
compatibility/bazel_tools/testing.bzl
"end": "2.4.0-snapshot.20220914.10592.0.cf7c2b5c",
"exclusions": [
"PartyManagementServiceIT:PMGetPartiesDetails",
"PartyManagementServiceIT:PMListKnownParties",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test tool exclusions

pbatko-da
pbatko-da commented Sep 28, 2022
View reviewed changes
...suites/src/main/scala/com/daml/ledger/api/testtool/suites/v1_8/UserManagementServiceIT.scala
Some(
User(
id = AdminUserId,
metadata = Some(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

'TestAdminExists' test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carrielaben-da carrielaben-da carrielaben-da left review comments

@remyhaemmerle-da remyhaemmerle-da remyhaemmerle-da left review comments

@da-tanabe da-tanabe da-tanabe left review comments

@mziolekda mziolekda mziolekda approved these changes

@stefanobaghino-da stefanobaghino-da Awaiting requested review from stefanobaghino-da

@hrischuk-da hrischuk-da Awaiting requested review from hrischuk-da

@ray-roestenburg-da ray-roestenburg-da Awaiting requested review from ray-roestenburg-da

@nickchapman-da nickchapman-da Awaiting requested review from nickchapman-da

@rgugliel-da rgugliel-da Awaiting requested review from rgugliel-da

@meiersi-da meiersi-da Awaiting requested review from meiersi-da

@garyverhaegen-da garyverhaegen-da Awaiting requested review from garyverhaegen-da

Assignees

@pbatko-da pbatko-da

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pbatko-da @da-tanabe @mziolekda @remyhaemmerle-da @carrielaben-da