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

DA5-5 persist/find draft transactions #1314

Merged
merged 1 commit into from
Oct 27, 2023
Merged

DA5-5 persist/find draft transactions #1314

merged 1 commit into from
Oct 27, 2023

Conversation

vlajos
Copy link
Contributor

@vlajos vlajos commented Oct 24, 2023 •
edited
Loading

Add UtxoLedgerService.persistDraftSignedTransaction and UtxoLedgerService.findDraftSignedTransaction

runtime-os: corda/corda-runtime-os#4964

github-actions[bot]
github-actions bot previously requested changes Oct 24, 2023
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

PR title failed to match regex -> ^((CORDA|EG|ENT|INFRA|CORE|DOC|ES)-\d+)(.*)

@vlajos vlajos changed the title DA5-5 persist/find draft transactions CORE-0000 DA5-5 persist/find draft transactions Oct 24, 2023
@github-actions github-actions bot dismissed their stale review October 24, 2023 13:44

All good!

@vlajos vlajos changed the title CORE-0000 DA5-5 persist/find draft transactions DA5-5 persist/find draft transactions Oct 24, 2023
github-actions[bot]
github-actions bot previously requested changes Oct 24, 2023
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

PR title failed to match regex -> ^((CORDA|EG|ENT|INFRA|CORE|DOC|ES|DA5-5)-\d+)(.*)

@vlajos vlajos force-pushed the vlajos/DA5-5-draft-tx branch from 38259d1 to 24d7fda Compare October 24, 2023 13:48
@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Oct 24, 2023 •
edited
Loading

Jenkins build for PR 1314 build 6

Build Successful:
Jar artifact version produced by this PR: 5.1.0.38-alpha-1698412635869

@vlajos vlajos changed the title DA5-5 persist/find draft transactions DA5-5-5 persist/find draft transactions Oct 24, 2023
@github-actions github-actions bot dismissed their stale review October 24, 2023 13:50

All good!

@vlajos vlajos changed the title DA5-5-5 persist/find draft transactions DA5-5 persist/find draft transactions Oct 24, 2023
@vlajos vlajos mentioned this pull request Oct 24, 2023
Closed
szymonsztuka
szymonsztuka previously approved these changes Oct 24, 2023
View reviewed changes
ledger/ledger-utxo/src/main/java/net/corda/v5/ledger/utxo/UtxoLedgerService.java
* @throws IllegalStateException if the transaction does not have any signatures.
*/
@Suspendable
void persistDraftSignedTransaction(
Copy link

@szymonsztuka szymonsztuka Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

draft transaction is term mostly used within the interop team, not sure if the wider audience will understand it (platfrom), alternatively it could be (maybe ugly name) persistPartiallySignedTransaction, both names for me are fine though

Copy link
Contributor

Choose a reason for hiding this comment

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

I think DraftTransaction is fine for the platform team as well.

ac101m
ac101m previously approved these changes Oct 24, 2023
View reviewed changes
Copy link
Contributor

@ac101m ac101m left a comment

Choose a reason for hiding this comment

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

LGTM

@vlajos vlajos marked this pull request as ready for review October 24, 2023 15:42
@vlajos vlajos requested a review from a team as a code owner October 24, 2023 15:42
blsemo
blsemo previously approved these changes Oct 24, 2023
View reviewed changes
Copy link
Contributor

@blsemo blsemo left a comment

Choose a reason for hiding this comment

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

LGTM

ledger/ledger-utxo/src/main/java/net/corda/v5/ledger/utxo/UtxoLedgerService.java
* @throws IllegalStateException if the transaction does not have any signatures.
*/
@Suspendable
void persistDraftSignedTransaction(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think DraftTransaction is fine for the platform team as well.

jmacmahonr3
jmacmahonr3 previously approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@jmacmahonr3 jmacmahonr3 left a comment

Choose a reason for hiding this comment

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

LGTM

@vlajos vlajos force-pushed the vlajos/DA5-5-draft-tx branch from 24d7fda to a0adc38 Compare October 27, 2023 12:30
@vlajos vlajos requested a review from a team as a code owner October 27, 2023 12:30
@vlajos vlajos changed the base branch from release/interop/syntax to release/c5-digital-assests October 27, 2023 12:31
@vlajos vlajos dismissed stale reviews from jmacmahonr3, blsemo, ac101m, and szymonsztuka October 27, 2023 12:31

The base branch was changed.

@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Oct 27, 2023 •
edited
Loading

Scanning for breaking API changes introduced by this PR

Scan Failed: https://ci02.dev.r3.com/job/Corda5/job/corda-api-compatibility/job/PR-1314/5/

If the breaking changes are intentional, run ./gradlew cementApi and get approval from the Corda team leads.

@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Oct 27, 2023 •
edited
Loading

Non-blocking downstream job failed for corda-non-functional-test

https://ci02.dev.r3.com/job/Corda5/job/corda-api-compatibility/job/PR-1314/5/ has failed for PR 1314 build 5

Please investigate if your changes may have broken compilation on https://github.com/corda/corda-non-functional-test

@corda-jenkins-ci02
Copy link
Contributor

Non-blocking downstream job failed for corda-e2e-test

https://ci02.dev.r3.com/job/Corda5/job/corda-api-compatibility/job/PR-1314/4/ has failed for PR 1314 build 4

Please investigate if your changes may have broken compilation on https://github.com/corda/corda-e2e-tests

@vlajos vlajos force-pushed the vlajos/DA5-5-draft-tx branch from a0adc38 to 83a2670 Compare October 27, 2023 13:16
@vlajos vlajos merged commit efa495b into release/c5-digital-assests Oct 27, 2023
@vlajos vlajos deleted the vlajos/DA5-5-draft-tx branch October 27, 2023 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szymonsztuka szymonsztuka szymonsztuka approved these changes

@github-actions github-actions[bot] github-actions[bot] left review comments

@blsemo blsemo blsemo left review comments

@driessamyn driessamyn Awaiting requested review from driessamyn

@jasonbyrner3 jasonbyrner3 Awaiting requested review from jasonbyrner3

@dimosr dimosr Awaiting requested review from dimosr

@ronanbrowne ronanbrowne Awaiting requested review from ronanbrowne

@rick-r3 rick-r3 Awaiting requested review from rick-r3

@simon-johnson-r3 simon-johnson-r3 Awaiting requested review from simon-johnson-r3

@Omar-awad Omar-awad Awaiting requested review from Omar-awad

@aditisdesai aditisdesai Awaiting requested review from aditisdesai

@vinir3 vinir3 Awaiting requested review from vinir3

@vkolomeyko vkolomeyko Awaiting requested review from vkolomeyko

@thiagoviana thiagoviana Awaiting requested review from thiagoviana

@Sakpal Sakpal Awaiting requested review from Sakpal

@ac101m ac101m Awaiting requested review from ac101m

@jmacmahonr3 jmacmahonr3 Awaiting requested review from jmacmahonr3

@julia-filipczak julia-filipczak Awaiting requested review from julia-filipczak

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

Successfully merging this pull request may close these issues.
5 participants
@vlajos @blsemo @szymonsztuka @ac101m @jmacmahonr3