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.

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

Tx metadata accepted in API for creating a transaction #2073

Closed
rvl opened this issue Aug 26, 2020 · 3 comments
Closed

Tx metadata accepted in API for creating a transaction #2073

rvl opened this issue Aug 26, 2020 · 3 comments
Assignees
Labels
ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG

Comments

@rvl
Copy link
Contributor

rvl commented Aug 26, 2020

Context

ADP-307

When creating a transaction, users should be able to attach their metadata in JSON format.

Decision

  1. Use json conversion functions from Cardano.Api.MetaData.

Acceptance Criteria

  1. The API must accept metadata when posting to the shelley create transaction endpoint.
  2. The API must accept metadata for the shelley transaction fee estimation endpoint.
  3. The metadata field must be optional.
  4. The swagger API definitions must be updated accordingly.

Development

  • As above.
  • Wrap TxMetadata in ApiTxMetadata so that JSON null can also denote absence of metadata.

QA

  1. Api type serialisation/deserialisation is tested with unit tests.
  2. There are unit tests in cardano-node for encoding/decoding metadata - see rework JSON conversions for transaction metadata IntersectMBO/cardano-node#1797. So we do not need to test multiple invalid metadata cases. One invalid metadata test is enough.
  3. Integration tests in lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Transactions.hs
    • TRANS_CREATE_01 is modified to check that metadata is missing.
    • TRANS_CREATE_10 - Transaction with metadata
    • TRANS_CREATE_11 - Transaction with invalid metadata -- this is a metadata parsing error.
    • TRANS_CREATE_12 - Transaction with too much metadata
@rvl rvl added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Aug 26, 2020
@rvl rvl added this to the (ADP-307) Transaction metadata milestone Aug 26, 2020
iohk-bors bot added a commit that referenced this issue Aug 28, 2020
2066: Bump cardano-addresses r=rvl a=Anviking

# Issue Number

Release.


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Bump cardano-addresses


# Comments

- I believe this purely affects CLI usage of the cardano-addresses binary that will get included in the wallet release

```
Bump includes:
- [Increase default mnemonic phrase length](IntersectMBO/cardano-addresses#53)
- [Allow to inspect reward accounts wrt #46](IntersectMBO/cardano-addresses#56)
- [Allow to generate stake addresses #58](IntersectMBO/cardano-addresses#58)
- [Make 'payment' subcommand generate testnet hrp, wrt #55](IntersectMBO/cardano-addresses#59)
```

<!-- 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
-->


2071: Minor nix cleanups r=rvl a=rvl

### Issue Number

#2046 #2054

### Overview

- Minor cleanups of nix code for migration tests and latency benchmark.

### Comments

Tested with:
```
nix-build -A benchmarks.cardano-wallet.latency
nix-build nix/migration-tests.nix
```


2080: Also trace the times of stake distribution LSQ queries r=rvl a=Anviking

# Issue Number

#2005 / new issue

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Also measure and trace the times of the LSQ queries in the `stakeDistribution` function


# 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
-->


2087: Transaction metadata in swagger API spec r=rvl a=rvl

### Issue Number

ADP-307 / #2073 / #2074 

### Overview

Updates the swagger spec for the new metadata field when:
- listing transactions (metadata field is always present but may be null)
- posting a transaction (metadata field is optional)
- estimating fee (as above)

### Comments

[Rendered spec](https://redocly.github.io/redoc/?url=https://raw.githubusercontent.com/input-output-hk/cardano-wallet/rvl/2073/swagger/specifications/api/swagger.yaml)


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
Co-authored-by: IOHK <devops+stack-project@iohk.io>
Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
@rvl rvl self-assigned this Aug 28, 2020
iohk-bors bot added a commit that referenced this issue Aug 28, 2020
2080: Also trace the times of stake distribution LSQ queries r=rvl a=Anviking

# Issue Number

#2005 / new issue

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Also measure and trace the times of the LSQ queries in the `stakeDistribution` function


# 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
-->


2087: Transaction metadata in swagger API spec r=rvl a=rvl

### Issue Number

ADP-307 / #2073 / #2074 

### Overview

Updates the swagger spec for the new metadata field when:
- listing transactions (metadata field is always present but may be null)
- posting a transaction (metadata field is optional)
- estimating fee (as above)

### Comments

[Rendered spec](https://redocly.github.io/redoc/?url=https://raw.githubusercontent.com/input-output-hk/cardano-wallet/rvl/2073/swagger/specifications/api/swagger.yaml)


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this issue Aug 28, 2020
2087: Transaction metadata in swagger API spec r=rvl a=rvl

### Issue Number

ADP-307 / #2073 / #2074 

### Overview

Updates the swagger spec for the new metadata field when:
- listing transactions (metadata field is always present but may be null)
- posting a transaction (metadata field is optional)
- estimating fee (as above)

### Comments

[Rendered spec](https://redocly.github.io/redoc/?url=https://raw.githubusercontent.com/input-output-hk/cardano-wallet/rvl/2073/swagger/specifications/api/swagger.yaml)


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this issue Aug 31, 2020
2087: Transaction metadata in swagger API spec r=KtorZ a=rvl

### Issue Number

ADP-307 / #2073 / #2074 

### Overview

Updates the swagger spec for the new metadata field when:
- listing transactions (metadata field is always present but may be null)
- posting a transaction (metadata field is optional)
- estimating fee (as above)

### Comments

[Rendered spec](https://redocly.github.io/redoc/?url=https://raw.githubusercontent.com/input-output-hk/cardano-wallet/rvl/2073/swagger/specifications/api/swagger.yaml)


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this issue Aug 31, 2020
2065: Fix occasional db tests failure r=KtorZ a=rvl

### Issue Number

Resolves #2064.

### Overview

The DB state machine tests found an extreme case which caused a `too many SQL variables` exception. Unfortunately it was not able to shrink the counterexample without crashing.

- [x] Changes cabal file so that tests are run with a memory limit of 2G.
- [x] Fixes the test case which sometimes fails.

### Comments

- Currently testing with:
   ```
   nix-build -A tests.cardano-wallet-core.unit
   cd lib/core
   while ../../result/bin/unit --match '/Cardano.Wallet.DB.Sqlite/Sqlite State machine tests'; do :; done
   ```
- There is an infinite loop in `Sqlite State machine tests`, it gets stuck somewhere, then takes quite a while to exhaust the heap:
  ```
        There's no checkpoint older than k (+/- 100)
          +++ OK, passed 100 tests.
    Sqlite State machine tests
  1594/100unit: Heap exhausted;
  unit: Current maximum heap size is 2147483648 bytes (2048 MB).
  unit: Use `+RTS -M<size>' to increase it.
   173,889,316,944 bytes allocated in the heap
     9,858,194,528 bytes copied during GC
     2,112,669,264 bytes maximum residency (720 sample(s))
        40,633,672 bytes maximum slop
              2014 MB total memory in use (0 MB lost due to fragmentation)

                                       Tot time (elapsed)  Avg pause  Max pause
    Gen  0     166050 colls,     0 par    7.239s   7.241s     0.0000s    0.0007s
    Gen  1       720 colls,     0 par   2585.867s  2752.903s     3.8235s    6.0089s

    TASKS: 5 (1 bound, 4 peak workers (4 total), using -N1)

    SPARKS: 0(0 converted, 0 overflowed, 0 dud, 0 GC'd, 0 fizzled)

    INIT    time    0.000s  (  0.000s elapsed)
    MUT     time  100.141s  ( 99.704s elapsed)
    GC      time  2593.106s  (2760.143s elapsed)
    EXIT    time    0.002s  (  0.003s elapsed)
    Total   time  2693.249s  (2859.850s elapsed)

    Alloc rate    1,736,438,027 bytes per MUT second

    Productivity   3.7% of total user, 3.5% of total elapsed
  ```
- After disabling shrinking on the state machine tests, the infinite loop turns into a failing test (with quite a large counterexample).
- There is a `too many variables` exception when querying the `tx_meta` table.


2087: Transaction metadata in swagger API spec r=KtorZ a=rvl

### Issue Number

ADP-307 / #2073 / #2074 

### Overview

Updates the swagger spec for the new metadata field when:
- listing transactions (metadata field is always present but may be null)
- posting a transaction (metadata field is optional)
- estimating fee (as above)

### Comments

[Rendered spec](https://redocly.github.io/redoc/?url=https://raw.githubusercontent.com/input-output-hk/cardano-wallet/rvl/2073/swagger/specifications/api/swagger.yaml)


2095: Replace BS.init with T.strip to handle windows newlines (`\r\n`) r=Anviking a=Anviking

# Issue Number

#2083 / related


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Replace BS.init with T.strip to handle windows newlines (`\r\n`). Integration tests failed with a pattern match failure here, when I ran locally on windows.


# Comments

- I've never seen CI fail for this reason, which is arguably strange. 

<!-- 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: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
iohk-bors bot added a commit that referenced this issue Sep 2, 2020
2089: Add TxMetadata to API r=KtorZ a=rvl

### Issue Number

ADP-307 / #2073 / #2074 

### Overview

Add TxMetadata to API.

### Comments

- Still in draft because API swagger tests are failing. It's not liking the schema references.

Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this issue Sep 2, 2020
2096: Tx metadata API integration tests r=KtorZ a=rvl

### Issue Number

ADP-307 / #2074 / #2073.

### Overview

- API test case for posting a transaction with metadata.
- Test creating a transaction with invalid metadata.
- Test creating a transaction that's too large due to metadata.
- Test that the transaction fee estimate is higher when there's metadata in the transaction.

### Comments

- Based on PR #2104 branch.

Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this issue Sep 2, 2020
2096: Tx metadata API integration tests r=KtorZ a=rvl

### Issue Number

ADP-307 / #2074 / #2073.

### Overview

- API test case for posting a transaction with metadata.
- Test creating a transaction with invalid metadata.
- Test creating a transaction that's too large due to metadata.
- Test that the transaction fee estimate is higher when there's metadata in the transaction.

### Comments

- Based on PR #2104 branch.

Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
@piotr-iohk
Copy link
Contributor

Perhaps this is still in progress, but just to share few observations.

It seems one can send metadata that is invalid in format, should we have more strict validation?

Examples:

  1. one can use the same key multiple times which restuls in silently using only first one... maybe there should be validation and such metadata not allowed?
    Metadata: {"1": 1, "1": 2, "1":123123} silently results in { "1": 1 }

  2. can successfuly send map which is not map (basically list of lists or more generally list of different types)

list of lists
{"4":[["k1", "v1", 1, 2, 4, 5, 6, 7], ["k2", "v2", "v3"]]}

list of different types
{111: [1,2,5,"4","4545", [1,2,4]]}

  1. can successfully send "nested" metadata
    { "0": {"cardano":"adasd", "2": {3: 4} } }

  2. too big top levels key is are silently made values that are OK (kind of cool, but maybe that's not OK?)

{"22222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222": 1, "222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222223434":123123}

👇

{
    "16397105843297379214": 1,
    "16397105843297380426": 123123
}

@KtorZ
Copy link
Member

KtorZ commented Sep 3, 2020

iohk-bors bot added a commit that referenced this issue Sep 21, 2020
2147: Use JSON "detailed schema" for API TxMetadata r=KtorZ a=rvl

### Issue Number

ADP-307 / #2073 

### Overview

Changes the API to use the "detailed" JSON schema for transaction metadata.

With this scheme, API users will always see exactly the same JSON in the transaction history and cardano explorer as what they submitted with the transaction.

### Comments

We may wish to support both formats in the API.

This PR is based on the branch of #2142.

Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
iohk-bors bot added a commit that referenced this issue Sep 22, 2020
2157: Revise swagger API documentation of TxMetadata r=KtorZ a=rvl

### Issue Number

ADP-307 / #2073

### Overview

Updates swagger documentation according to recent API changes.

### Comments

[Rendered](https://redocly.github.io/redoc/?url=https://raw.githubusercontent.com/input-output-hk/cardano-wallet/rvl/2073/tx-metadata-swagger/specifications/api/swagger.yaml)




Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
iohk-bors bot added a commit that referenced this issue Sep 22, 2020
2157: Revise swagger API documentation of TxMetadata r=KtorZ a=rvl

### Issue Number

ADP-307 / #2073

### Overview

Updates swagger documentation according to recent API changes.

### Comments

[Rendered](https://redocly.github.io/redoc/?url=https://raw.githubusercontent.com/input-output-hk/cardano-wallet/rvl/2073/tx-metadata-swagger/specifications/api/swagger.yaml)




Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this issue Sep 22, 2020
2157: Revise swagger API documentation of TxMetadata r=KtorZ a=rvl

### Issue Number

ADP-307 / #2073

### Overview

Updates swagger documentation according to recent API changes.

### Comments

[Rendered](https://redocly.github.io/redoc/?url=https://raw.githubusercontent.com/input-output-hk/cardano-wallet/rvl/2073/tx-metadata-swagger/specifications/api/swagger.yaml)




Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
@piotr-iohk
Copy link
Contributor

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG
Projects
None yet
Development

No branches or pull requests

3 participants