-
Notifications
You must be signed in to change notification settings - Fork 217
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
simple metadata format in some transaction endpoints #3253
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @paolino
Many thanks for making this PR!
I've not had a chance to look through all the functionality yet, but there are a couple of things to fix:
-
The latency benchmark currently doesn't build, because
Link.getTransaction
now takes an extra argument. If you're typically building withcabal build all
, then you might want to runcabal configure --enable-benchmarks
. This will make sure thatcabal build all
also builds the benchmarks! (I was also caught out by this the other day...) -
There are some import and formatting issues that we should fix.
I created PR #3257 with suggested fixes for both of these issues. If you're okay with the commits in that PR, please feel free to squash them into your own branch, or cherry-pick them if you prefer. 👍🏻
Thanks @jonathanknowles for the fixes! |
User-facing changes
|
e0daedf
to
afecfb1
Compare
bors try |
tryBuild failed:
|
bors try |
tryBuild failed:
|
bors r+ |
👎 Rejected by code reviews |
bors try |
tryBuild failed:
|
I cannot reproduce these failures locally (unless I run the whole suite, individually they pass). I'm going to keep bors trying for a bit. Perhaps this is a resource issue? |
Hi @paolino Regarding:
As promised, I've made these proposed changes in #3270. |
…simple-metadata Use `TxMetadataSchema` in preference to `Bool` for function parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
But not to CI ! |
this passes on |
tryBuild failed: |
lib/core-integration/src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs
Outdated
Show resolved
Hide resolved
lib/core-integration/src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs
Outdated
Show resolved
Hide resolved
…/Transactions.hs Co-authored-by: Heinrich Apfelmus <[email protected]>
fix ((bors r+) >>) |
bors r+ |
3253: simple metadata format in some transaction endpoints r=piotr-iohk a=paolino - We implement support for transaction creation (old and new) - we implement a query parameter `simple-metadata` on `get transaction` and `list transactions` to select the new format in the response - we implement a switch in the CLI commands get and list to support the new format in the output - we use `oneOf ` openapi combinator to document the possibility of the two formats for the same endpoints ### Issue Number ADP-1596 3269: Validity interval fix r=paweljakubas a=paweljakubas <!-- Detail in a few bullet points the work accomplished in this PR. Before you submit, don't forget to: * Make sure the GitHub PR fields are correct: ✓ Set a good Title for your PR. ✓ Assign yourself to the PR. ✓ Assign one or more reviewer(s). ✓ Link to a Jira issue, and/or other GitHub issues or PRs. ✓ In the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. * Don't waste reviewers' time: ✓ If it's a draft, select the Create Draft PR option. ✓ Self-review your changes to make sure nothing unexpected slipped through. * Try to make your intent clear: ✓ Write a good Description that explains what this PR is meant to do. ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding Jira ticket. ✓ Highlight what Testing you have done. ✓ Acknowledge any changes required to the Documentation. --> - [x] simplyfying interval logic - [x] enhaning integration testing ### Comments <!-- Additional comments, links, or screenshots to attach, if any. --> ### Issue Number adp-1738 <!-- Reference the Jira/GitHub issue that this PR relates to, and which requirements it tackles. Note: Jira issues of the form ADP- will be auto-linked. --> Co-authored-by: Paolo Veronelli <[email protected]> Co-authored-by: Paolo Veronelli <[email protected]> Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: Piotr Stachyra <[email protected]>
Seems build timed out but bors didn't notice 🤔 |
bors r- |
Canceled. |
bors r+ |
3253: simple metadata format in some transaction endpoints r=sevanspowell a=paolino - We implement support for transaction creation (old and new) - we implement a query parameter `simple-metadata` on `get transaction` and `list transactions` to select the new format in the response - we implement a switch in the CLI commands get and list to support the new format in the output - we use `oneOf ` openapi combinator to document the possibility of the two formats for the same endpoints ### Issue Number ADP-1596 3269: Validity interval fix r=paweljakubas a=paweljakubas <!-- Detail in a few bullet points the work accomplished in this PR. Before you submit, don't forget to: * Make sure the GitHub PR fields are correct: ✓ Set a good Title for your PR. ✓ Assign yourself to the PR. ✓ Assign one or more reviewer(s). ✓ Link to a Jira issue, and/or other GitHub issues or PRs. ✓ In the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. * Don't waste reviewers' time: ✓ If it's a draft, select the Create Draft PR option. ✓ Self-review your changes to make sure nothing unexpected slipped through. * Try to make your intent clear: ✓ Write a good Description that explains what this PR is meant to do. ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding Jira ticket. ✓ Highlight what Testing you have done. ✓ Acknowledge any changes required to the Documentation. --> - [x] simplyfying interval logic - [x] enhaning integration testing ### Comments <!-- Additional comments, links, or screenshots to attach, if any. --> ### Issue Number adp-1738 <!-- Reference the Jira/GitHub issue that this PR relates to, and which requirements it tackles. Note: Jira issues of the form ADP- will be auto-linked. --> Co-authored-by: Paolo Veronelli <[email protected]> Co-authored-by: Paolo Veronelli <[email protected]> Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: Piotr Stachyra <[email protected]>
Build failed (retrying...): |
3253: simple metadata format in some transaction endpoints r=sevanspowell a=paolino - We implement support for transaction creation (old and new) - we implement a query parameter `simple-metadata` on `get transaction` and `list transactions` to select the new format in the response - we implement a switch in the CLI commands get and list to support the new format in the output - we use `oneOf ` openapi combinator to document the possibility of the two formats for the same endpoints ### Issue Number ADP-1596 Co-authored-by: Paolo Veronelli <[email protected]> Co-authored-by: Paolo Veronelli <[email protected]> Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]>
Build failed: |
bors r+ |
3253: simple metadata format in some transaction endpoints r=sevanspowell a=paolino - We implement support for transaction creation (old and new) - we implement a query parameter `simple-metadata` on `get transaction` and `list transactions` to select the new format in the response - we implement a switch in the CLI commands get and list to support the new format in the output - we use `oneOf ` openapi combinator to document the possibility of the two formats for the same endpoints ### Issue Number ADP-1596 Co-authored-by: Paolo Veronelli <[email protected]> Co-authored-by: Paolo Veronelli <[email protected]> Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]>
Build failed: |
Will merge manually, in accordance with decision recorded here: https://md.adrestia.iohkdev.io/s/jo5Of1Qli# |
simple-metadata
onget transaction
andlist transactions
to select the new format in the responseoneOf
openapi combinator to document the possibility of the two formats for the same endpointsUser facing changes
Issue Number
ADP-1596