ADP-455: return unsigned delegation certificate #2213

hasufell · 2020-10-05T18:51:04Z

Issue Number

#2200

Overview

Comments

lib/core/src/Cardano/Wallet.hs

lib/core/src/Cardano/Wallet/Transaction.hs

lib/core/src/Cardano/Wallet/Api/Types.hs

lib/core/src/Cardano/Wallet/Api.hs

hasufell · 2020-10-12T18:01:38Z

bors try

hasufell · 2020-10-12T18:19:33Z

This is ready for review, except for the missing integration tests... @piotr-iohk anything specific you want covered?

iohk-bors · 2020-10-12T18:53:31Z

try

Build failed:

buildkite/cardano-wallet

hasufell · 2020-10-12T19:07:14Z

bors try

iohk-bors · 2020-10-12T19:44:54Z

try

Build failed:

ci/hydra-build:required

hasufell · 2020-10-12T19:49:44Z

bors try

iohk-bors · 2020-10-12T20:41:53Z

try

Build failed:

buildkite/cardano-wallet

hasufell · 2020-10-12T21:46:22Z

bors try

iohk-bors · 2020-10-12T22:35:24Z

try

Build failed:

buildkite/cardano-wallet

hasufell · 2020-10-14T13:23:05Z

@piotr-iohk I added all integration tests and fixed the errors. But I'm still not 100% sure the output is what we want.

hasufell · 2020-10-14T14:24:07Z

bors try

iohk-bors · 2020-10-14T15:32:27Z

try

Build succeeded:

specifications/api/swagger.yaml

KtorZ · 2020-10-15T14:31:10Z

specifications/api/swagger.yaml

+  type: array
+  minItems: 1
+  items:
+    type: string


Can't this re-use the derivationIndex schema instead?

There is no such schema?

x-derivationSegment: &derivationSegment description: An individual segment within a derivation path. type: string example: 1852H x-derivationPath: &derivationPath description: A path for deriving a child key from a parent key. type: array minItems: 1 items: *derivationSegment

*derivationSegment / derivationPath.

specifications/api/swagger.yaml

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

lib/core/src/Cardano/Wallet/Api/Server.hs

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/StakePools.hs

hasufell · 2020-10-15T16:41:08Z

@KtorZ could you reply to #2213 (comment) since we're still not sure the output is actually correct

KtorZ · 2020-10-16T11:33:16Z

The output looks quite correct to me. For a first registration, we would expect two certificates, and they are qualified as expected.

piotr-iohk · 2020-10-16T11:48:17Z

Minor thing:

But trying with hex pool_id we're getting:

curl  -vX POST http://localhost:8090/v2/wallets/1ceb45b37a94c7022837b5ca14045f11a5927c65/coin-selections/random \
  -H "Content-Type: application/json; charset=utf-8" \
  -d '{
"delegation_action": 
{
	"action": "join",
	"pool":"9f36c4c67b76a8cea05a1684f8feaa64711f4c0053fe039978b203af"
} 
}' | jq

👇

{ "code": "bad_request",
  "message":
         "Error in $['delegation_action'].pool: Invalid stake pool id: expecting a Bech32 encoded value with human readable part of 'pool'."}

hasufell · 2020-10-16T11:53:24Z

The output looks quite correct to me. For a first registration, we would expect two certificates, and they are qualified as expected.

I was more worried about the amounts in input and output...

The code was becoming quite convoluted here with function names that were screaming for refactoring (when we start adding `'` to functions, it's usually a good sign that something can be simplified). The main change is actually in 'selectCoinsExternal' which is now parameterized over the selection to run. This way, the steps of assigning change addresses are factored out in this function and a lot of the duplication goes away. Another important change is that I've moved the signing and submission of join/quit outside of the body of the function. So that each step can be ran independently. This avoid the need for weird intermediate product types aggregating more and more information. Now, both functions are returning a 'DelegationAction' and merely checking that a join or quit is possible.

select coins for delegation simplifications

KtorZ · 2020-10-16T14:40:49Z

@hasufell there's one input selected, and a single change output. The delta is: 2 174 433, which corresponds I believe to 2Ada of key deposit, and ~0.17 Ada for fees which seems right for the Testnet.

KtorZ · 2020-10-16T14:55:38Z

bors merge

piotr-iohk · 2020-10-16T16:11:54Z

Playing with the branch I observe that for the wallet that is already delegating to a pool, invoking a join action mostly results in:

{"code":"unable_to_assign_input_output"
,"message":"I'm unable to assign outputs from coin selection: inputs: - 1st eefa06df (~ 1000000 @ 00b69bf0...bca0a380) withdrawal: -0 reclaim: -0 outputs: [] change: [] deposit: -0 "}

Roughly 1 out of 5 is successful. The rest is this 👆 .

For the case when wallet is not delegating... all seems to be OK.

cc: @hasufell @KtorZ

iohk-bors · 2020-10-16T16:16:07Z

Build succeeded:

KtorZ · 2020-10-18T09:29:41Z

@piotr-iohk on which commit did you do these tests 🤔 ?

piotr-iohk · 2020-10-18T10:32:12Z

@KtorZ the last one from this branch. The same is actually now on master. -> 2020.10.13 (git revision: 05e59a0)

E.g.: I'm seeing this on testnet for the wallet: vacant cheap ghost elegant silly square offer vault announce turtle lesson doll key canyon romance which is delegating to pool1frevxe70aqw2ce58c0muyesnahl88nfjjsp25h85jwakzgd2g2l.

curl  -vX POST http://localhost:8090/v2/wallets/1b0aa24994b4181e79116c131510f2abf6cdaa4f/coin-selections/random \
  -H "Content-Type: application/json; charset=utf-8" \
  -d '{
"delegation_action": 
{
	"action": "join",
	"pool":"pool1numvf3nmw65vagz6z6z03l42v3c37nqq20lq8xtckgp67zzl7px"
} 
}' | jq

👇
most of the time:

{
  "code": "unable_to_assign_input_output",
  "message": "I'm unable to assign outputs from coin selection: inputs: - 1st eefa06df (~ 1000000 @ 00b69bf0...bca0a380) withdrawal: -0 reclaim: -0 outputs: [] change: [] deposit: -0 "
}

sometimes:

{
  "inputs": [
    {
      "amount": {
        "quantity": 497657470,
        "unit": "lovelace"
      },
      "address": "addr1qp5aaw59aaa5z82dgvmpn7xryejcn9j00plsv32y4gw6veqhpgwhzykzha0z6w27gxsps40gurvxjt2vxh26009q5wqqmy4ryk",
      "id": "601dd956ade7e3aefdb2618889c05508ced1515711b5ed8da1a8cd495033004b",
      "derivation_path": [
        "1852H",
        "1815H",
        "0H",
        "1",
        "1"
      ],
      "index": 0
    }
  ],
  "certificates": [
    {
      "certificate_type": "join_pool",
      "pool": "pool1numvf3nmw65vagz6z6z03l42v3c37nqq20lq8xtckgp67zzl7px",
      "reward_account_path": [
        "1852H",
        "1815H",
        "0H",
        "2",
        "0"
      ]
    }
  ],
  "outputs": [
    {
      "amount": {
        "quantity": 497484709,
        "unit": "lovelace"
      },
      "address": "addr1qpdunk6q5uw5uwnvptttn7htzqms0amj5ztv98npdekn7aghpgwhzykzha0z6w27gxsps40gurvxjt2vxh26009q5wqq7rgr20"
    }
  ]
}

@piotr-iohk

2255: do not enforce non-empty outputs for unsigned tx r=KtorZ a=KtorZ # Issue Number  #2200 # Overview  - c60c5a6 📍 **do not enforce non-empty outputs for unsigned tx** Actually, there are many use-cases for empty outputs. This occurs in particular often when performing a selection for delegation: in this scenario, there are typically no outputs at all and, depending on the size of the selected input(s), there might be no change at all (because of the minUTxO value). # Comments  Should fix: - #2213 (comment) - #2213 (comment) Thanks @piotr-iohk 🙏  Co-authored-by: KtorZ <[email protected]>

@piotr-iohk

2255: do not enforce non-empty outputs for unsigned tx r=KtorZ a=KtorZ # Issue Number  #2200 # Overview  - c60c5a6 📍 **do not enforce non-empty outputs for unsigned tx** Actually, there are many use-cases for empty outputs. This occurs in particular often when performing a selection for delegation: in this scenario, there are typically no outputs at all and, depending on the size of the selected input(s), there might be no change at all (because of the minUTxO value). # Comments  Should fix: - #2213 (comment) - #2213 (comment) Thanks @piotr-iohk 🙏  Co-authored-by: KtorZ <[email protected]>

hasufell self-assigned this Oct 5, 2020

hasufell added the Feature Mark a PR as adding a new feature, for auto-generated CHANGELOG label Oct 5, 2020

hasufell linked an issue Oct 5, 2020 that may be closed by this pull request

Return unsigned delegation certificate #2200

Closed

hasufell added this to the (ADP-455) return unsigned delegation certificate milestone Oct 5, 2020

hasufell marked this pull request as draft October 5, 2020 18:52

hasufell force-pushed the hasufell/2200/unsigned-delegation-cert branch from 7aa3cc1 to 2798019 Compare October 5, 2020 19:03

KtorZ suggested changes Oct 6, 2020

View reviewed changes

hasufell force-pushed the hasufell/2200/unsigned-delegation-cert branch 2 times, most recently from dec15ff to 4b91fea Compare October 9, 2020 15:40

hasufell commented Oct 9, 2020

View reviewed changes

lib/core/src/Cardano/Wallet/Api.hs Outdated Show resolved Hide resolved

hasufell force-pushed the hasufell/2200/unsigned-delegation-cert branch from c50b8b1 to 4238bda Compare October 12, 2020 18:00

hasufell changed the title ~~[WIP] ADP-455: return unsigned delegation certificate~~ ADP-455: return unsigned delegation certificate Oct 12, 2020

hasufell marked this pull request as ready for review October 12, 2020 18:01

iohk-bors bot added a commit that referenced this pull request Oct 12, 2020

Try #2213:

9477157

hasufell requested a review from KtorZ October 12, 2020 18:20

hasufell force-pushed the hasufell/2200/unsigned-delegation-cert branch from de668bb to 8dcf5af Compare October 12, 2020 19:04

iohk-bors bot added a commit that referenced this pull request Oct 12, 2020

Try #2213:

7d572a9

iohk-bors bot added a commit that referenced this pull request Oct 12, 2020

Try #2213:

0c8f382

hasufell force-pushed the hasufell/2200/unsigned-delegation-cert branch from c17bedc to 881fab6 Compare October 12, 2020 21:39

iohk-bors bot added a commit that referenced this pull request Oct 12, 2020

Try #2213:

9f9c4dd

hasufell force-pushed the hasufell/2200/unsigned-delegation-cert branch from 881fab6 to b5f8caf Compare October 13, 2020 08:44

Julian Ospald added 2 commits October 14, 2020 15:02

Assign change addresses in quitStakePoolUnsigned

7616b5c

Add integration tests

1fc3ac4

iohk-bors bot added a commit that referenced this pull request Oct 14, 2020

Try #2213:

91bb123

KtorZ reviewed Oct 15, 2020

View reviewed changes

Apply further review suggestions

9b379d2

hasufell force-pushed the hasufell/2200/unsigned-delegation-cert branch from c4a154b to 9b379d2 Compare October 15, 2020 17:27

Fix 'Arbitrary ApiCertificate'

16451e5

hasufell force-pushed the hasufell/2200/unsigned-delegation-cert branch from df33084 to 16451e5 Compare October 16, 2020 11:27

KtorZ and others added 3 commits October 16, 2020 15:31

return structured error for invalid wallets, instead of plain string

4a5f4d9

Merge pull request #2254 from input-output-hk/KtorZ/2200/simplifications

759fd0e

select coins for delegation simplifications

KtorZ approved these changes Oct 16, 2020

View reviewed changes

iohk-bors bot merged commit 05e59a0 into master Oct 16, 2020

iohk-bors bot deleted the hasufell/2200/unsigned-delegation-cert branch October 16, 2020 16:16

KtorZ mentioned this pull request Oct 19, 2020

do not enforce non-empty outputs for unsigned tx #2255

Merged

ADP-455: return unsigned delegation certificate #2213

ADP-455: return unsigned delegation certificate #2213

Uh oh!

Conversation

hasufell commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Number

Overview

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hasufell commented Oct 12, 2020

Uh oh!

hasufell commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iohk-bors bot commented Oct 12, 2020

try

Uh oh!

hasufell commented Oct 12, 2020

Uh oh!

iohk-bors bot commented Oct 12, 2020

try

Uh oh!

hasufell commented Oct 12, 2020

Uh oh!

iohk-bors bot commented Oct 12, 2020

try

Uh oh!

hasufell commented Oct 12, 2020

Uh oh!

iohk-bors bot commented Oct 12, 2020

try

Uh oh!

hasufell commented Oct 14, 2020

Uh oh!

hasufell commented Oct 14, 2020

Uh oh!

iohk-bors bot commented Oct 14, 2020

try

Uh oh!

Uh oh!

Uh oh!

KtorZ Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

hasufell Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

KtorZ Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hasufell commented Oct 15, 2020

Uh oh!

KtorZ commented Oct 16, 2020

Uh oh!

piotr-iohk commented Oct 16, 2020

Uh oh!

hasufell commented Oct 16, 2020

Uh oh!

KtorZ commented Oct 16, 2020

Uh oh!

KtorZ commented Oct 16, 2020

hasufell commented Oct 5, 2020 •

edited

Loading

hasufell commented Oct 12, 2020 •

edited

Loading