API data-types & specifications for arbitrary metadata signing. #2257
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
added
the
ADDING FEATURE
KtorZ
commented
Oct 19, 2020
| Just s -> | ||
| pure s | ||
| parseJSON = parseJSON | ||
| >=> fmap ApiT . eitherToParser . first ShowFmt . fromText |
There was a problem hiding this comment.
Parsing delegated to the FromText instance and moved near the data-type declaration.
KtorZ
commented
Oct 19, 2020
| where | ||
| firstHardened = getIndex @'Hardened minBound | ||
|
|
||
| toJSON = toJSON . toText . getApiT |
There was a problem hiding this comment.
Serialization delegated to the ToText instance and moved near the data-type declaration.
KtorZ
force-pushed
the
KtorZ/ADP-508/metadata-api-data-types
branch
from
da28646 to
e03e3fe
Compare
KtorZ
commented
Oct 19, 2020
|
|
||
| > ⚠️ On the incentivized testnet, this parameter is not requred, but rather completely ignored. | ||
| > ⚠️ On the incentivized testnet, this parameter is not requred, but rather completely ignored. |
There was a problem hiding this comment.
This was somewhat indented too far. Sorry for the noise.
rvl
approved these changes
Oct 20, 2020
Comment on lines
+172
to
+173
| = UtxoExternal | ||
| | UtxoInternal |
There was a problem hiding this comment.
I think this could be fixed in fromTextToBoundedEnum SnakeLowerCase.
It's spelt UTxO everywhere else.
This important word deserves special case treatment, so that we don't need to mangle our type names just to get the preferred serialisation.
There was a problem hiding this comment.
I thought of both options and thought that it was cleaner in the end to rename the type here. It's perhaps a little less pervasive as a change to just deal with it in the toText//fromText instances. Any way requires a database fix / migration / work-around :|
|
|
||
| paths: | ||
| /wallets/{walletId}/{role}/{index}/signatures: |
I had to also move it in '...Primitive.AddressDerivation' because of otherwise circular dependencies (since this module is using the Bounded instances of 'Index', relying on the 'DerivationType' as type parameter. This is mostly extracting the text parsing that was already done in the JSON instance, and moving it as a ToText/FromText instances (and re-using it to build the JSON instances). I've however added a quick sanity text roundtrip property test in the meantime.
We use the data-type constructor in order to generate text instances.
However, our automatic casing function is doing pretty bad with the
mixed cases and we end up with something like:
- u_tx_o_external
- u_tx_o_internal
- mutable_account
Instead of fixing the instance itself to work around this, I found it
easier to just fix the type (which isn't used in many places).
This is a rather bad practice; if a value failed to parse, then we have very little idea about _what_ it is and what's it's shape. It may not be even readable / printable. Instead, the error message should show valid expected options.
KtorZ
force-pushed
the
KtorZ/ADP-508/metadata-api-data-types
branch
from
e03e3fe to
fb5ab0d
Compare
|
bors merge |
iohk-bors bot
added a commit
that referenced
this pull request
Oct 20, 2020
2257: API data-types & specifications for arbitrary metadata signing. r=KtorZ a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> ADP-481 > ADP-508 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - 209781b 📍 **define ToText/FromText instances for 'DerivationIndex'** I had to also move it in '...Primitive.AddressDerivation' because of otherwise circular dependencies (since this module is using the Bounded instances of 'Index', relying on the 'DerivationType' as type parameter. This is mostly extracting the text parsing that was already done in the JSON instance, and moving it as a ToText/FromText instances (and re-using it to build the JSON instances). I've however added a quick sanity text roundtrip property test in the meantime. - c965936 📍 **rename UTxO*ternal -> Utxo*ternal** We use the data-type constructor in order to generate text instances. However, our automatic casing function is doing pretty bad with the mixed cases and we end up with something like: - u_tx_o_external - u_tx_o_internal - mutable_account Instead of fixing the instance itself to work around this, I found it easier to just fix the type (which isn't used in many places). - c4cbafe 📍 **do not output back invalid input when parsing text values.** This is a rather bad practice; if a value failed to parse, then we have very little idea about _what_ it is and what's it's shape. It may not be even readable / printable. Instead, the error message should show valid expected options. - fb5ab0d 📍 **extend API and specifications to include the 'signMetadata' operation** # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <[email protected]>
|
Build failed: plus a few more similar looking failures |
|
Fixed the integration test - baffling failure message though. bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Oct 21, 2020
2257: API data-types & specifications for arbitrary metadata signing. r=rvl a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> ADP-481 > ADP-508 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - 209781b 📍 **define ToText/FromText instances for 'DerivationIndex'** I had to also move it in '...Primitive.AddressDerivation' because of otherwise circular dependencies (since this module is using the Bounded instances of 'Index', relying on the 'DerivationType' as type parameter. This is mostly extracting the text parsing that was already done in the JSON instance, and moving it as a ToText/FromText instances (and re-using it to build the JSON instances). I've however added a quick sanity text roundtrip property test in the meantime. - c965936 📍 **rename UTxO*ternal -> Utxo*ternal** We use the data-type constructor in order to generate text instances. However, our automatic casing function is doing pretty bad with the mixed cases and we end up with something like: - u_tx_o_external - u_tx_o_internal - mutable_account Instead of fixing the instance itself to work around this, I found it easier to just fix the type (which isn't used in many places). - c4cbafe 📍 **do not output back invalid input when parsing text values.** This is a rather bad practice; if a value failed to parse, then we have very little idea about _what_ it is and what's it's shape. It may not be even readable / printable. Instead, the error message should show valid expected options. - fb5ab0d 📍 **extend API and specifications to include the 'signMetadata' operation** # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]>
|
Build failed: hlint, |
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Oct 21, 2020
2244: Add derivation paths to coin selection change outputs r=jonathanknowles a=jonathanknowles # Issue Number #2247 # Overview This PR introduces a new `ApiCoinSelectionChange` type that includes a derivation path: ```hs data ApiCoinSelectionChange (n :: NetworkDiscriminant) = ApiCoinSelectionChange { address :: !(ApiT Address, Proxy n) , amount :: !(Quantity "lovelace" Natural) , derivationPath :: NonEmpty (ApiT DerivationIndex) } deriving (Eq, Generic, Show) ``` In addition, we modify the `ApiCoinSelection` type so that ordinary outputs (**_without_** derivation paths) and change outputs (**_with_** derivation paths) are held in separate fields: ```patch data ApiCoinSelection (n :: NetworkDiscriminant) = ApiCoinSelection { inputs :: !(NonEmpty (ApiCoinSelectionInput n)) - , outputs :: !(NonEmpty (AddressAmount (ApiT Address, Proxy n))) + , outputs :: !(NonEmpty (ApiCoinSelectionOutput n)) + , change :: ![ApiCoinSelectionChange n] } deriving (Eq, Generic, Show) ``` # Comments This PR updates the following integration tests: - `WALLETS_COIN_SELECTION_01`. - `WALLETS_COIN_SELECTION_02`. - `SHELLEY_HW_WALLETS/HW_WALLETS_04`. - `BYRON_HW_WALLETS/HW_WALLETS_04`. 2257: API data-types & specifications for arbitrary metadata signing. r=rvl a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> ADP-481 > ADP-508 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - 209781b 📍 **define ToText/FromText instances for 'DerivationIndex'** I had to also move it in '...Primitive.AddressDerivation' because of otherwise circular dependencies (since this module is using the Bounded instances of 'Index', relying on the 'DerivationType' as type parameter. This is mostly extracting the text parsing that was already done in the JSON instance, and moving it as a ToText/FromText instances (and re-using it to build the JSON instances). I've however added a quick sanity text roundtrip property test in the meantime. - c965936 📍 **rename UTxO*ternal -> Utxo*ternal** We use the data-type constructor in order to generate text instances. However, our automatic casing function is doing pretty bad with the mixed cases and we end up with something like: - u_tx_o_external - u_tx_o_internal - mutable_account Instead of fixing the instance itself to work around this, I found it easier to just fix the type (which isn't used in many places). - c4cbafe 📍 **do not output back invalid input when parsing text values.** This is a rather bad practice; if a value failed to parse, then we have very little idea about _what_ it is and what's it's shape. It may not be even readable / printable. Instead, the error message should show valid expected options. - fb5ab0d 📍 **extend API and specifications to include the 'signMetadata' operation** # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: KtorZ <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]>
|
This PR was included in a batch that timed out, it will be automatically retried |
iohk-bors bot
added a commit
that referenced
this pull request
Oct 21, 2020
2257: API data-types & specifications for arbitrary metadata signing. r=rvl a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> ADP-481 > ADP-508 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - 209781b 📍 **define ToText/FromText instances for 'DerivationIndex'** I had to also move it in '...Primitive.AddressDerivation' because of otherwise circular dependencies (since this module is using the Bounded instances of 'Index', relying on the 'DerivationType' as type parameter. This is mostly extracting the text parsing that was already done in the JSON instance, and moving it as a ToText/FromText instances (and re-using it to build the JSON instances). I've however added a quick sanity text roundtrip property test in the meantime. - c965936 📍 **rename UTxO*ternal -> Utxo*ternal** We use the data-type constructor in order to generate text instances. However, our automatic casing function is doing pretty bad with the mixed cases and we end up with something like: - u_tx_o_external - u_tx_o_internal - mutable_account Instead of fixing the instance itself to work around this, I found it easier to just fix the type (which isn't used in many places). - c4cbafe 📍 **do not output back invalid input when parsing text values.** This is a rather bad practice; if a value failed to parse, then we have very little idea about _what_ it is and what's it's shape. It may not be even readable / printable. Instead, the error message should show valid expected options. - fb5ab0d 📍 **extend API and specifications to include the 'signMetadata' operation** # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]>
|
Build failed: |
|
bors retry |
|
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.
Issue Number
ADP-481 > ADP-508
Overview
209781b
📍 define ToText/FromText instances for 'DerivationIndex'
I had to also move it in '...Primitive.AddressDerivation' because of otherwise circular dependencies (since this module is using the Bounded instances of 'Index', relying on the 'DerivationType' as type parameter. This is mostly extracting the text parsing that was already done in the JSON instance, and moving it as a ToText/FromText instances (and re-using it to build the JSON instances). I've however added a quick sanity text roundtrip property test in the meantime.
c965936
📍 rename UTxOternal -> Utxoternal
We use the data-type constructor in order to generate text instances.
However, our automatic casing function is doing pretty bad with the
mixed cases and we end up with something like:
Instead of fixing the instance itself to work around this, I found it
easier to just fix the type (which isn't used in many places).
c4cbafe
📍 do not output back invalid input when parsing text values.
This is a rather bad practice; if a value failed to parse, then we have very little idea about what it is and what's it's shape. It may not be even readable / printable. Instead, the error message should show valid expected options.
fb5ab0d
📍 extend API and specifications to include the 'signMetadata' operation
Comments