BACK-6974: New schema for EIP712 #171
Conversation
There was a problem hiding this comment.
Can you please add a description to your PR referencing specs for proper historization ? Thanks!
Adding diff of the schema for clarity:
❯ difft ethereum/eip712.schema.json ethereum/eip712-v2.schema.json back6974
ethereum/eip712-v2.schema.json --- JSON
34 34 "properties": {
.. 35 "assetPath": {
.. 36 "type": "string"
.. 37 },
.. 38 "format": {
.. 39 "enum": [
.. 40 "coin",
.. 41 "amount",
.. 42 "address"
.. 43 ]
.. 44 },
35 45 "label": {There was a problem hiding this comment.
I believe if we go with option 3 as specified in BACK-6974, the new schema file should replace the old one ?
There was a problem hiding this comment.
The option 3 is about the asset file itself, not its schema. I think all schemas should remain in input repositories, and the single asset file should meet all these schema (this is why we cannot authorize to drop or alter field, nor add mandatory fields)
There was a problem hiding this comment.
Documents can't pass all schemas, as additionalProperties is set to false
There was a problem hiding this comment.
Change should be applied in all networks
There was a problem hiding this comment.
As stated here, these changes are meant to be used by app-eth. Do we really need to impact non-ethereum network?
There was a problem hiding this comment.
All these networks are EVMs, handled by the ethereum app
| "type": "string" | ||
| }, | ||
| "format": { | ||
| "enum": [ |
There was a problem hiding this comment.
We also need a raw value for unformatted fields, see https://github.com/LedgerHQ/app-ethereum/blob/feat/apa/eip712_improvements/doc/ethapp.adoc#show-raw-field
There was a problem hiding this comment.
yes, I missed it since I relied on https://ledgerhq.atlassian.net/wiki/spaces/PTX/pages/4404903999/ARCHI+EIP-712+improvements which does not match the code you show me.
ethereum/eip712-v2.schema.json
Outdated
| "enum": [ | ||
| "coin", | ||
| "amount", | ||
| "address" |
There was a problem hiding this comment.
address will not be implemented in this version, let's remove it (see https://github.com/LedgerHQ/app-ethereum/blob/feat/apa/eip712_improvements/doc/ethapp.adoc#eip712-filtering)
ethereum/eip712-v2.schema.json
Outdated
| }, | ||
| "format": { | ||
| "enum": [ | ||
| "coin", |
There was a problem hiding this comment.
I suggest we follow the nomenclature defined on the nano app for clarity: https://github.com/LedgerHQ/app-ethereum/blob/feat/apa/eip712_improvements/doc/ethapp.adoc#eip712-filtering
| "coin", | |
| "token", |
| "type": "string" | ||
| }, | ||
| "format": { | ||
| "enum": [ |
There was a problem hiding this comment.
No description provided.