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

MVP Multi Domain Ledger API #16760

Merged
merged 57 commits into from
Jun 4, 2023
Merged

MVP Multi Domain Ledger API #16760

merged 57 commits into from
Jun 4, 2023

Conversation

nmarton-da
Copy link
Contributor

@nmarton-da nmarton-da commented Apr 24, 2023 •
edited
Loading

This PR introduces Ledger API V2 in a backwards compatible fashion.
For change-log and migration hints please read v2/package.proto.

cocreature
cocreature approved these changes Apr 25, 2023
View reviewed changes
Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

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

Nice mostly looks good, I'm unclear on the semantics of not passing domain ids. Do I get daml 2.0 behavior on reads? What happens on command submissions? I'm also slightly worried about the oddities in the flat transaction stream but that may be workable for june.

ledger-api/grpc-definitions/com/daml/ledger/api/v1/command_completion_service.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/commands.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/transfer.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/update_service.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/update_service.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/update_service.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/in_flight_transfers_service.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/in_flight_transfers_service.proto Outdated Show resolved Hide resolved
meiersi-da
meiersi-da reviewed May 2, 2023
View reviewed changes
Copy link
Contributor

@meiersi-da meiersi-da left a comment

Choose a reason for hiding this comment

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

Added the comments from the review as prep to the call today. Please ping me when the next iteration has completed and is ready for another review.

ledger-api/grpc-definitions/com/daml/ledger/api/v1/active_contracts_service.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/connected_domains_service.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/connected_domains_service.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/in_flight_transfers_service.proto Outdated Show resolved Hide resolved
cocreature
cocreature reviewed May 16, 2023
View reviewed changes
ledger-api/grpc-definitions/com/daml/ledger/api/v1/active_contracts_service.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/active_contracts_service.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/active_contracts_service.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/commands.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/commands.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/connected_domains_service.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/in_flight_transfers_service.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/completion.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/active_contracts_service.proto Outdated Show resolved Hide resolved
ledger-api/grpc-definitions/com/daml/ledger/api/v1/active_contracts_service.proto Outdated Show resolved Hide resolved
@rgugliel-da
Copy link
Contributor

General question: do we want to be able to filter for a list of domains? (ACS snapshot, update stream, ...)

@nmarton-da
Extend to be revisited list
e8d2388 
[CHANGELOG_BEGIN]
[CHANGELOG_END]
@nmarton-da nmarton-da marked this pull request as ready for review June 4, 2023 11:36
@nmarton-da nmarton-da requested review from a team as code owners June 4, 2023 11:36
@nmarton-da
Add new proto-s to test-protobuf-structure.sh
20d2a0a 
[CHANGELOG_BEGIN]
[CHANGELOG_END]
@nmarton-da nmarton-da merged commit 9f55d67 into main Jun 4, 2023
@nmarton-da nmarton-da deleted the nmarton/mvp-multi-domain-api branch June 4, 2023 21:00
remyhaemmerle-da added a commit that referenced this pull request Jun 5, 2023
@remyhaemmerle-da
Reactivate test disable in #16760 except on windows
645762d 
namely

- //daml-script/export/integration-tests/reproduces-transactions:test
- //compiler/damlc:daml-stdlib-doctest
remyhaemmerle-da added a commit that referenced this pull request Jun 5, 2023
@remyhaemmerle-da
Reactivate test disable in #16760 except on windows (#16957)
ce2203d 
namely

- //daml-script/export/integration-tests/reproduces-transactions:test
- //compiler/damlc:daml-stdlib-doctest
remyhaemmerle-da added a commit that referenced this pull request Jun 5, 2023
@remyhaemmerle-da
Reactivate windows tests disable in #16760
f4b5f4a 
namely:
-  //daml-script/export/integration-tests/reproduces-transactions:test
-  //compiler/damlc:daml-stdlib-doctest
andreaslochbihler-da
andreaslochbihler-da reviewed Jun 5, 2023
View reviewed changes
ledger-api/grpc-definitions/com/daml/ledger/api/v2/participant_offset.proto
// one with a bigger participant offset happened after than the one with a smaller participant
// offset. Please note this is not true for updates synchronized by a different domain.
//
// The Ledger API endpoints that take offsets allow to specify portions
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you mean by "portion" here. Is that defined somewhere else in the API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nah, this is the original doc which I kept around

andreaslochbihler-da
andreaslochbihler-da reviewed Jun 5, 2023
View reviewed changes
ledger-api/grpc-definitions/com/daml/ledger/api/v2/participant_offset.proto
}

enum ParticipantBoundary {
// Refers to the first transaction.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to slot in a "currently" like for end, as pruning can move the "first" and it's not clear that there is no pruning happen concurrently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to me from the implementation that this is actually fetching from the beginning...this also meas that this is not usable for stream start after the first pruning.
I am not sure this is practical, but I would not change the doc here, because the current implementation is aligned with the current doc.

rgugliel-da
rgugliel-da reviewed Jun 7, 2023
View reviewed changes
ledger-api/grpc-definitions/com/daml/ledger/api/v2/state_service.proto
message GetLedgerEndResponse {

// The absolute offset of the current ledger end.
ParticipantOffset offset = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently (research), Canton always return an absolute offset. Do we want to change that ?

Copy link
Contributor Author

@nmarton-da nmarton-da Jun 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good point: this is part of the "to-be-revisited" list already: https://github.com/digital-asset/daml/blob/main/ledger-api/grpc-definitions/com/daml/ledger/api/v2/README.md#to-be-revisited-potential-changes-until-daml-sdk-30ga #3

remyhaemmerle-da added a commit that referenced this pull request Jun 7, 2023
@remyhaemmerle-da
Reactivate windows tests disable in #16760
0041c11 
namely:
-  //daml-script/export/integration-tests/reproduces-transactions:test
-  //compiler/damlc:daml-stdlib-doctest
@nmarton-da nmarton-da mentioned this pull request Jun 7, 2023
Merged
rgugliel-da
rgugliel-da reviewed Jun 13, 2023
View reviewed changes
ledger-api/grpc-definitions/com/daml/ledger/api/v2/transaction_filter.proto

// A filter both for filtering create and archive events as well as for
// filtering transaction trees.
message TransactionFilter {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late question but why "TransactionFilter" ? We use it for other things: updates, acs

garyverhaegen-da pushed a commit that referenced this pull request Jun 13, 2023
@remyhaemmerle-da @garyverhaegen-da
Reactivate windows tests disable in #16760
fc17693 
namely:
-  //daml-script/export/integration-tests/reproduces-transactions:test
-  //compiler/damlc:daml-stdlib-doctest
garyverhaegen-da added a commit that referenced this pull request Jun 20, 2023
@remyhaemmerle-da @garyverhaegen-da
Reactivate windows tests disable in #16760 (#16958)
7b7a6f6 
namely:
-  //daml-script/export/integration-tests/reproduces-transactions:test
-  //compiler/damlc:daml-stdlib-doctest

Co-authored-by: Gary Verhaegen <gary.verhaegen@digitalasset.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@waynecollier-da waynecollier-da waynecollier-da left review comments

@rgugliel-da rgugliel-da rgugliel-da approved these changes

@stefanobaghino-da stefanobaghino-da stefanobaghino-da approved these changes

@andreaslochbihler-da andreaslochbihler-da andreaslochbihler-da left review comments

@mziolekda mziolekda mziolekda left review comments

@meiersi-da meiersi-da meiersi-da left review comments

@cocreature cocreature cocreature approved these changes

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

@basvangijzel-DA basvangijzel-DA Awaiting requested review from basvangijzel-DA

@remyhaemmerle-da remyhaemmerle-da Awaiting requested review from remyhaemmerle-da

@akrmn akrmn Awaiting requested review from akrmn

@dylant-da dylant-da Awaiting requested review from dylant-da

@samuel-williams-da samuel-williams-da Awaiting requested review from samuel-williams-da

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@nmarton-da @rgugliel-da @cocreature @meiersi-da @mziolekda @andreaslochbihler-da @stefanobaghino-da @waynecollier-da