Remove 'coin_selection' field from the balance API's response #2964
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KtorZ
commented
Oct 11, 2021
| @@ -996,7 +996,7 @@ data PostTransactionFeeOldData (n :: NetworkDiscriminant) = PostTransactionFeeOl | |||
|
|
|||
| type ApiBase64 = ApiBytesT 'Base64 ByteString | |||
|
|
|||
| newtype ApiSignedTransaction = ApiSignedTransaction | |||
| newtype ApiSerializedTransaction = ApiSerializedTransaction | |||
There was a problem hiding this comment.
Renamed since it's no longer only used for the sign endpoint, but also as a result of the balance endpoint.
There was a problem hiding this comment.
maybe it is worth to align this with https://github.com/input-output-hk/cardano-wallet/blob/master/specifications/api/swagger.yaml#L2217
specifications/api/swagger.yaml
Outdated
| ApiSignedTransaction: &ApiSignedTransaction | ||
| description: | | ||
| The result of signing a transaction (serialized and encoded). | ||
| ApiSerializedTransaction: &ApiSerializedTransaction |
There was a problem hiding this comment.
we have also this https://github.com/input-output-hk/cardano-wallet/blob/master/specifications/api/swagger.yaml#L2217
One letter difference 's' -> 'z'. Would be good to reconcile
There was a problem hiding this comment.
ah, they are the same. Both use serialisedTransactionBase64
There was a problem hiding this comment.
Arg! Indeed. UK vs US. Seems like the API uses the UK's style more, so I'll go with that.
KtorZ
force-pushed
the
KtorZ/ADP-1183/cleanup-balance-api
branch
from
624ce26 to
876dc62
Compare
| @@ -996,7 +996,7 @@ data PostTransactionFeeOldData (n :: NetworkDiscriminant) = PostTransactionFeeOl | |||
|
|
|||
| type ApiBase64 = ApiBytesT 'Base64 ByteString | |||
|
|
|||
| newtype ApiSignedTransaction = ApiSignedTransaction | |||
| newtype ApiSerialisedTransaction = ApiSerialisedTransaction | |||
There was a problem hiding this comment.
so the idea is to use in response of balanceTx, and in response of signTx ApiSerialisedTransaction? and they are going to be base64 encoded bytestring (signTx will also accept hex coming from constructTx), the same as in case of request of submitTx. Right?
There was a problem hiding this comment.
Note that this doesn't change anything compared to what was already there w.r.t to the serialised transaction returned by the API, it however removes the coin_selection field from the balance's result. For the rest, it's identical.
paweljakubas
approved these changes
Oct 11, 2021
KtorZ
force-pushed
the
KtorZ/ADP-1183/balancing-execution-units
branch
2 times, most recently
from
45d239c to
8305b23
Compare
Base automatically changed from
KtorZ/ADP-1183/balancing-execution-units
to
master
October 11, 2021 11:54
KtorZ
force-pushed
the
KtorZ/ADP-1183/cleanup-balance-api
branch
from
876dc62 to
8a2ac2b
Compare
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Oct 11, 2021
2964: Remove 'coin_selection' field from the balance API's response r=KtorZ a=KtorZ
<!--
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.
-->
- 📍 **Remove 'coin_selection' field from the Balance API's response**
(a) the implementation of this field was wrong, and getting it right is quite involved as it requires revising several parts of the core wallet. It is perhaps not desirable / out-of-scope for this endpoint actually.
(b) the coin_selection result is only truly useful when constructing a transaction which is under control of the wallet. Balancing may be done on any arbitrary foreign transaction and as such, breaks several invariants of the wallet (only one withdrawal, only one delegation certificate at a time etc..).
### Comments
<!-- Additional comments, links, or screenshots to attach, if any. -->
Based off [`KtorZ/ADP-1183/balancing-execution-units`](#2952)
### Issue Number
<!-- 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. -->
ADP-1183
Co-authored-by: KtorZ <[email protected]>
|
Build failed: |
KtorZ
force-pushed
the
KtorZ/ADP-1183/cleanup-balance-api
branch
from
8a2ac2b to
ef1fdcc
Compare
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Oct 11, 2021
2964: Remove 'coin_selection' field from the balance API's response r=KtorZ a=KtorZ
<!--
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.
-->
- 📍 **Remove 'coin_selection' field from the Balance API's response**
(a) the implementation of this field was wrong, and getting it right is quite involved as it requires revising several parts of the core wallet. It is perhaps not desirable / out-of-scope for this endpoint actually.
(b) the coin_selection result is only truly useful when constructing a transaction which is under control of the wallet. Balancing may be done on any arbitrary foreign transaction and as such, breaks several invariants of the wallet (only one withdrawal, only one delegation certificate at a time etc..).
### Comments
<!-- Additional comments, links, or screenshots to attach, if any. -->
Based off [`KtorZ/ADP-1183/balancing-execution-units`](#2952)
### Issue Number
<!-- 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. -->
ADP-1183
Co-authored-by: KtorZ <[email protected]>
|
Build failed: |
(a) the implementation of this field was wrong, and getting it right is quite involved as it requires revising several parts of the core wallet. It is perhaps not desirable / out-of-scope for this endpoint actually. (b) the coin_selection result is only truly useful when constructing a transaction which is under control of the wallet. Balancing may be done on any arbitrary foreign transaction and as such, breaks several invariants of the wallet (only one withdrawal, only one delegation certificate at a time etc..).
KtorZ
force-pushed
the
KtorZ/ADP-1183/cleanup-balance-api
branch
from
ef1fdcc to
37f6a97
Compare
|
bors r+ |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📍 Remove 'coin_selection' field from the Balance API's response
(a) the implementation of this field was wrong, and getting it right is quite involved as it requires revising several parts of the core wallet. It is perhaps not desirable / out-of-scope for this endpoint actually.
(b) the coin_selection result is only truly useful when constructing a transaction which is under control of the wallet. Balancing may be done on any arbitrary foreign transaction and as such, breaks several invariants of the wallet (only one withdrawal, only one delegation certificate at a time etc..).
Comments
Based off
KtorZ/ADP-1183/balancing-execution-unitsIssue Number
ADP-1183