Impl signing for minting/burning in new tx flow

[paweljakubas]

• cleaning old minting/burning enpoint
• add postPolicyKey endpoint used when the already existent wallet does not have policy xpub
• add signing
• Adjust fee calculation
• show signing in integration testing
• add optional param to POST policy key
• unify construct/decode mint/burn responses

Comments

Minting/burning working without timelocks at this moment - needs validation integral fixing to enable this - coming in the next PR

I want to have construct/decode responses:

{
  "tokens": [
    {
      "policy_id": "0fc891fb7368d3b7c7b88815c203fda0d6862b0f1d797222672e91fe",
      "policy_script": "policy_vkh1ecqfpeunygcgl4tqqycn86x6mhna99vsrm5pk22vq8dvxfq3htn", 
      "assets" : [
           { "asset_name": "1111",
             "quantity": 1,
             "fingerprint": "asset17jwl24z89xcgljv57fe4de558zvwl6k9h0hf22" 
           }, 
           { "asset_name": "2222",
             "quantity": 10,
             "fingerprint": "asset17jwl24z89xcgljv57fe4de558zvwl66666666" 
           }, 
      ]
    }
  ],
  "wallet_policy_key_index": "0H",
  "wallet_policy_key_hash": "policy_vkh1ecqfpeunygcgl4tqqycn86x6mhna99vsrm5pk22vq8dvxfq3htn"
}

Issue Number

adp-955
adp-1542

[piotr-iohk]

Suggested change

	Create policy key for the wallet. Used for public account key derived
	wallets or already instantiated mnemonic wallets to set policy public key
	Create policy key for the wallet.
	In order to be able to mint/burn assets with `POST Construct` endpoint there needs to be
	a policy key set for the wallet. Invoking this endpoint would be required for all wallets instantiated
	before introducing mint/burn feature prior to making a mint/burn transaction from them.

[piotr-iohk]

I guess that this endpoint now invalidates https://input-output-hk.github.io/cardano-wallet/api/edge/#operation/getPolicyKey... I mean there's basically no need for it, right?

Also postPolicyKey returns actually a hash (policy_vkh), perhaps we should add a query parameter hash=true here and return policy_vk or policy_vkh based on it?

[paweljakubas]

now, for the consistency I added optional parameter like in getPolicyKey.
I would advocate for maintaining getPolicyKey in line with postPolicyKey. Could be helpful for consumers of the API, also POST writes to DB, and GET is reading

[piotr-iohk]

Ok, sounds good to leave getPolicyKey, but perhaps for postPolicyKey the POST should be actually PUT... it doesn't create new one every time, but sets one if doesn't exists (or overwrites the old one, but it is always the same if I'm not mistaken)

[sevanspowell]

Thanks for this work @paweljakubas. This kind of work which involves making changes from the lowest level to the highest is difficult.

I've read through everything and it seems sensible. However, just like the last PR I reviewed it is hard to give proper review.

For me, being able to read and understand the tests is more important than the code and it's performance characteristics. In my review I can't assert that:

• All edge cases are being tested.
• Tests do indeed test what they're trying to test.

I have two requests:

1. Is it possible to structure the code so that it can be property tested? I'd be really interested in property tests that cover the edge cases of common attack vectors like burning tokens you don't have, minting and burning at the same time, etc.
   • Structuring the code such that it can be property tested may make it easier to review too.
2. Could we raise the level of abstraction in the integration tests? It's becoming increasingly difficult to untangle the core of the integration tests from the code (i.e. "with this input, I expect this output").
   • Even just some high-level functions (no need to make a new DSL) could help here.

I understand you're just working with what you've got, and these aren't issues you've introduced, but I can't give meaningful review without a clearer relationship between the code and the proof that code works.

[paweljakubas]

hi @sevanspowell ,

Thanks for looking at the PR.
Several things were solved here:

1. adding postPolicyKey endpoint - needed for already existing wallets to participate in minting/burning
2. work improving what decodeTransaction emit when minting/burning is included.
3. signing. and here we have several parts:

• detect during signTransaction that there is minting/burning with our policy_keys - here we learn about our policy key by callign getPolicyKey
• if we need to add the policy key private key, then we derive it and sneak it in addVkWitnesses
• on shelley level we use this key and create shelley witness, here we try not to be redundant if possible
• some changes were needed in also fee skeleton

4. integration tests can't be much simpler imho. I have now two backbones, one for minting, one for burning. And they are reused in several tests, for example, when showing burning the minting part is reused. And each part just checks all expectations from construct - signing - submitting - decode and finally when looking at balance and seeing presence of assets or absence.

I plan to un-pend the pending tests in the forthcoming PR. I will also think about testing this during property testing. The next PRs will be very focused. This one is big as I needed to prove that minting/burning signing really works in integration testing

[piotr-iohk]

Is it possible to structure the code so that it can be property tested? I'd be really interested in property tests that cover the edge cases of common attack vectors like burning tokens you don't have, minting and burning at the same time, etc.
Structuring the code such that it can be property tested may make it easier to review too.

If this is possible that would be awesome. I think testing all cases on integration level is a bit of overkill so if there are any cases that could be verified with property tests that would be great. Then integration tests can be more about e2e flow rather than edge cases.

Just to note, besides the integration tests that are pending here and cover some of the edge cases (too big amount when minting, to long asset_name when minting) I have discovered following issues while working on e2e tests in #3207:

• minting to receiving_address doesn't work. Minted tokens always land on source wallet balance,
• minting and burning in single transaction doesn't work, submitting returns:

{
   "code":"created_invalid_transaction",
   "message":"The submitted transaction was rejected by the local node. Here's an error message that may help with debugging: TxValidationErrorInMode (ShelleyTxValidationError ShelleyBasedEraAlonzo (ApplyTxError [UtxowFailure (WrappedShelleyEraFailure (ScriptWitnessNotValidatingUTXOW (fromList [ScriptHash \"971551a411b40f2ab6636e94e5404e0ef496b58cf2074effc957e4b5\",ScriptHash \"be37b36b712c083bc6e4fdf81f665e315131c3057ec552ab62f20ea8\"])))])) AlonzoEraInCardanoMode"
}

[paweljakubas]

@sevanspowell @piotr-iohk @jonathanknowles yes, we can try to address all edge cases in next PR. This PR is adding the backbone of signing, plus e2e integration tests showing he case. This is first interaction out of three needed to have it in production. I am in favor of merging it, and then (1) improve edge cases via adding more integration tests and property tests, (2) fixing validity interval to enable timelocks. Please take notice that this affects only new tx workflow which is STILL experimental. Otherwise, we could spend many days getting (1) married with some signing/fee/server/wallet bits. Let's merge this and address (1) and (2) in next PRs and move on.

[jonathanknowles]

Hi @paweljakubas

I've left some comments and questions!

[jonathanknowles]

Hi Pawel

Many thanks for creating this PR!

I think it's going in the right direction. However, I do share the concerns that @sevanspowell expressed above, namely that:

Is it possible to structure the code so that it can be property tested? I'd be really interested in property tests that cover the edge cases of common attack vectors like burning tokens you don't have, minting and burning at the same time, etc.

I also feel we should invest more time in writing some focussed property tests, in addition to the integration tests you've already written.

I feel the main advantage would be that we can generate hundreds of random examples, rather than the very small number we're able to test in our integration test framework. We'd also gain a shorter editing-testing iteration cycle, which means faster feedback when things break. Lastly, it might help us to break code up into smaller, clearer chunks, which would answer Sam's point about making the code easier to review.

At the end of the day, as you pointed out, we do still need the integration tests to prove that the system works from end-to-end, but I feel this should be more of a "sanity check" rather than the main thrust of our test effort.

Perhaps we can add more property testing in the next PR? In particular, I'm curious to verify what happens in the following edge cases, in addition to what Sam already mentioned:

• minting and burning in the same transaction (disjoint sets of assets)
• minting and burning in the same transaction (intersecting sets of assets: this might provoke some failures)
• attempting to burn assets that are not available
• attempting to mint or burn large numbers of assets (so that the map is very large: I wonder whether we'll hit upon some kind of limit).

I suspect there are also lots of other things we can test.

Anyway, I'm happy to approve, but I do think it would also be good to also get a review from someone who is more knowledgeable about what kind of design we really want for the new transaction API. (I can only review this part very superficially!) Perhaps @rvl has an opinion here.

Thanks again!

Jonathan

[paweljakubas]

thanks @jonathanknowles a lot for the review and commits. I will address those points in the forthcoming PR!

[paweljakubas]

bors r+

[iohk-bors]

Build succeeded:

• buildkite/cardano-wallet
• ci/hydra-build:required