ADP-494 - Add POST endpoint for script address #2253

paweljakubas · 2020-10-16T11:11:00Z

Issue Number

https://jira.iohk.io/browse/ADP-494

Overview

I have updated swagger with script as string
I have added script as json
I have extended Api
I have extended core unit tests
I have added cardano-addresses based impl
I have added illustrative integration tests

Comments

Uses Add pub option for public keys IntersectMBO/cardano-addresses#87

paweljakubas · 2020-10-16T11:19:49Z

specifications/api/swagger.yaml

@@ -610,6 +640,16 @@ x-signedTransactionBlob: &signedTransactionBlob
  type: string
  format: binary

+x-addressParts: &addressParts


enterprise address -> { networkTag, paymentCredential }
reward account address -> { networkTag, stakeCredential }
stake address -> { networkTag, paymentCredential, stakeCredential }

KtorZ · 2020-10-16T11:18:19Z

specifications/api/swagger.yaml

+x-networkTag: &networkTag
+  description: Can be null for 'Icarus' and 'Byron' styles.
+  type: integer
+  minimum: 0


This should not be necessary because the server is already instantiated for one of the given networks. So the network tag is implicit based on how one started the server (--mainnet or --testnet)

removed in ccd4eb4

KtorZ · 2020-10-16T11:19:00Z

specifications/api/swagger.yaml

@@ -321,6 +326,30 @@ x-poolMetadataSource: &poolMetadataSource
      format: uri
      example: https://smash.cardano-mainnet.iohk.io/

+x-walletCredentialPubkey: &walletCredentialPubkey
+  description: An extended public key (public key + chain code) for credential


Why require an extended key here? The chain code is unnecessary.

to my best knowledge we are hashing extended public key with blake2b not just public key -> https://github.com/input-output-hk/cardano-addresses/blob/master/core/lib/Cardano/Address/Style/Shelley.hs#L639

No we're not :)

https://github.com/input-output-hk/cardano-addresses/blob/1939121afcd125b6c6300a1b2a8b014007a0356c/core/lib/Cardano/Address/Style/Shelley.hs#L869-L872

xpubPublicKey is the public key, without chain code.

indeed! Thanks! I made this mistake mainly because of this :

$ cat signingKey1.prv | cardano-address key public --bech32 xpub13pfp60uf4ru7cmj5d57xjdzj6lagn328ah67h35urqrl92jn094z0s07fcy2pvfw3xfhna6fa24na8nk6lraymjcrl5wwj6chsjqtjqlfw44z

So we will need a way to get public key only (not chain code included) in cardano-addresses, right?

I think that'd be an interesting feature indeed. Be able to get just the public part, without chain code

Alternatively, we can have the API accepts both, but the chain code should not a required

corrected in 2f91803

KtorZ · 2020-10-16T11:19:08Z

specifications/api/swagger.yaml

+x-walletCredentialPubkey: &walletCredentialPubkey
+  description: An extended public key (public key + chain code) for credential
+  type: string
+  format: hex


What about bech32 ;) ?

used xpub hrp -> ccd4eb4

KtorZ · 2020-10-16T11:19:55Z

specifications/api/swagger.yaml

+
+x-walletScript: &walletScript
+  description: A script as text with predicates and verification keys. Here probably we should add a full-fledged script object
+  type: string


How is that a string 🤔 ? Isn't the goal here to re-use the JSON object instances from cardano-addresses?

it is going to be json object. just doing this in steps

my attempt -> ccd4eb4
But when

$ openapi-spec-validator --schema 3.0.0 specifications/api/swagger.yaml found undefined alias 'walletCredentialScript' in "<file>", line 357, column 14

need to recall how to sneak recursive scheme in swagger

Look at transaction metadatum. I recall we did something similar.

thanks, now it seems to be ok -> 2f91803

KtorZ · 2020-10-22T14:50:09Z

stack.yaml

@@ -87,177 +87,177 @@ nix:
 # package cardano-crypto
 #   tests: False
 #   benchmarks: False
-# 
+#


I think that stack2cabal relies on these whitespaces and they shouldn't be trimmed.

@paweljakubas, is your editor correctly picking up the config from: https://github.com/input-output-hk/cardano-wallet/blob/master/.editorconfig#L11-L12

it seems so - this is config that is taken by my editor for this file:

# EditorConfig for /home/pawel/Work/IOHK/cardano-wallet/stack.yaml indent_style = space indent_size = 2 end_of_line = lf charset = utf-8 trim_trailing_whitespace = false insert_final_newline = true tab_width = 2

I think trim_trailling_whitespace=true is a nice thing to have.

When auto-generated whitespace gets removed in a file that you edited, use your editor's git function to discard those irrelevant hunks.

KtorZ · 2020-10-22T14:50:46Z

stack.yaml

@@ -46,7 +46,7 @@ extra-deps:

 # cardano-addresses-2.1.0
 - git: https://github.com/input-output-hk/cardano-addresses
-  commit: e0ab4587266430a08734e1aec1c29d261a9a3b70
+  commit: 1399751501b03346265b2e429e5832bacd6df279


We should only pick released version of cardano-addresses. But I am quite reluctant releasing it in the state it is right now :/ ... I'd rather have it compliant with CIP5 for the release.

agree, pasted it as it is just in order to have a way to interact with what we have in cardano-addresses

paweljakubas · 2020-10-27T13:42:28Z

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

+      netTag = case testEquality (typeRep @n) (typeRep @'Mainnet) of
+          Just{}  -> 1
+          Nothing -> 0
+--}


This one is escaping me -> I want n -> Int ... for now I hacked it to be 1 (Mainnet) to carry on with integration tests

Note that this should not be in core, because it only concerns cardano-node and really is cardano-node specific. Plus, you should be able to simply re-use: https://github.com/input-output-hk/cardano-wallet/blob/368b9fc27249e1faf6577c020297b74d4cf6f161/lib/shelley/src/Cardano/Wallet/Shelley.hs#L203-L218

thanks for this hint. did it in 3cf22f4 also did some reshuffling of code connected with HasNetworkId to use it

KtorZ · 2020-10-27T14:33:02Z

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

+    { payload :: ByteString
+    , flavour :: AnyAddressType
+    , network :: Int
+    } deriving (Eq, Generic, Show)


Why not re-use the existing types for Addresses ?

If I use Addresses I will be able to differentiate between addr and addr_test based on network. but I will not be able to use reward_account's stake and stake_test (to my best knowledge). This was main reason I introduced this type

KtorZ · 2020-10-27T14:33:43Z

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

+    } deriving (Eq, Generic, Show)
+
+data Credential = Credential
+    { credential :: Either ApiPubKey ApiScript


May I suggest a sum-type instead of wrapping a Either ?

data Credential = CredentialPubKey ... | CredentialScript

?

Should it be called ApiCredential as well for consistency?

done in 1cdaf25

KtorZ · 2020-10-27T14:39:07Z

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

+                    pure ( unAddress $
+                           CA.delegationAddress discriminant (spendingFromScript script1) (stakingFromScript script2)
+                         , EnterpriseDelegating )
+                (Nothing, Nothing) -> fail "At least one credential is required"


That's one big chunk 😬
My recommendation would be to capture this already in the API data-type itself. So, instead of:

data ApiCredentials = ApiCredentials { spending :: !(Maybe Credential) , staking :: !(Maybe Credential) } deriving (Eq, Generic, Show)

have something along the lines of:

data ApiAddressData = AddrBase Credential Credential | AddrPtr Credential Ptr | AddrEnterprise Credential | AddrBootstrap Address

this would be quite more readable and also get rid of the "impossible" case.

good idea, indeed it made things simpler -> 1071e78

rvl

Good start 👍

rvl · 2020-10-28T03:55:59Z

specifications/api/swagger.yaml

+     - any condition for which any list element' expectation is to be met for the condition to be valid
+     - some condition for which a minimal number of list elements' expectation is to be met for the condition to be valid
+    example:
+      0: "script_vkh18srsxr3khll7vl3w9mqfu55n6wzxxlxj7qzr2mhnyreluzt36ms"


This value should be a yaml list of examples, rather than an object with numeric keys. The single example object is specific to Metadata.

done in e6c0510

rvl · 2020-10-28T03:58:57Z

specifications/api/swagger.yaml

+      properties:
+        address: *anyAddress
+
+    ApiCredential: &ApiCredential


Could this be renamed to ScriptAddressCredential? Are the Api prefixes needed for anything? This is a swagger file - it's all API.

@rvl we typically keep the same naming as in the Cardano.Wallet.Api.Types module for the swagger file references because it creates a straightforward mapping for the automated schema validations we perform in tests. This way, one can write ToSchema instances as:

instance ToSchema (ApiSelectCoinsPayments n) where declareNamedSchema _ = declareSchemaForDefinition "ApiSelectCoinsPayments" instance ToSchema ApiSelectCoinsAction where declareNamedSchema _ = declareSchemaForDefinition "ApiSelectCoinsAction" instance ToSchema (ApiCoinSelection n) where declareNamedSchema _ = declareSchemaForDefinition "ApiCoinSelection" instance ToSchema ApiWallet where declareNamedSchema _ = declareSchemaForDefinition "ApiWallet" instance ToSchema ApiByronWallet where declareNamedSchema _ = declareSchemaForDefinition "ApiByronWallet"

We could even push it further and have default instances relying on generics and Typeable.

Aha ok thanks.

rvl · 2020-10-28T04:02:40Z

specifications/api/swagger.yaml

+    ApiScript: &ApiScript
+      type: object
+      required:
+        - script
+      properties:
+        script: *script
+
+    ApiPubKey: &ApiPubKey
+      type: object
+      required:
+        - pub_key
+      properties:
+        pub_key: *credentialPubKey


I think these could both be removed and embedded directly into CredentialValue

I wanted them to be exposed to have a way in TypeSpec to check

OK fair enough

rvl · 2020-10-28T04:06:58Z

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

@@ -384,6 +398,34 @@ data ApiAddress (n :: NetworkDiscriminant) = ApiAddress
    , state :: !(ApiT AddressState)
    } deriving (Eq, Generic, Show)

+newtype ApiScript = ApiScript
+    { script :: ApiT Script


I think if the sole field is going to be ApiT Script, then you don't need the ApiScript newtype.

Perhaps the newtype might be needed if you want automatic JSON instances, but it looks like they are non-generically defined below.

rvl · 2020-10-28T04:08:01Z

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

+    } deriving (Eq, Generic, Show)
+
+data Credential = Credential
+    { credential :: Either ApiPubKey ApiScript


Should it be called ApiCredential as well for consistency?

rvl · 2020-10-28T04:12:51Z

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

+    arbitrary = ApiScript . ApiT <$> arbitrary
+
+instance Arbitrary Script where
+    arbitrary = do


Same comment about generators of arbitrary tree structures applies here as it did in cardano-addresses.

In fact, could the types and generators from cardano-addresses be re-used?

did in e6c0510

rvl · 2020-10-28T04:16:34Z

stack.yaml

@@ -87,177 +87,177 @@ nix:
 # package cardano-crypto
 #   tests: False
 #   benchmarks: False
-# 
+#


I think trim_trailling_whitespace=true is a nice thing to have.

When auto-generated whitespace gets removed in a file that you edited, use your editor's git function to discard those irrelevant hunks.

rvl · 2020-10-28T04:18:42Z

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

+postAnyAddress
+    :: forall n. ApiCredentials
+    -> Handler AnyAddress
+postAnyAddress body = do


It looks like not just any address can be posted. Is that right?

yes, I can construct enterprise, reward account and base address for any credential (either from key or script). There is also pointer address missing that could be added in forthcoming PR. That's the rationale I proposed the name AnyAddress

piotr-iohk · 2020-10-30T14:30:31Z

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

+    -- Generating golden test data for enterprise addresses:
+    --- $ cardano-address script hash "$(cat script.txt)" \
+    --- | cardano-address address payment --from-script --network-tag mainnet
+    it "ANY_ADDRESS_POST_01 - Golden tests for enterprise script address - signature" $ \ctx -> do


Nitpicking but why golden? Afaik, golden are unit tests with output in separate file which is not the case here.

golden because I have generated golden data in cardano-address for all examples, and check if all json/handling/.. machinery comes to the same results

piotr-iohk · 2020-10-30T14:38:56Z

specifications/api/swagger.yaml

+                                          , "script_vkh18srsxr3khll7vl3w9mqfu55n6wzxxlxj7qzr2mhnyreluzt38ms"]
+                               , "at_least" : 2
+                               }
+                    } ] }


There seems to be some formatting issue here:

thanks, will fix

paweljakubas · 2020-10-30T15:39:06Z

I added golden for enterprise, reward account and base with script(s) as credential. I will add more (probably in separate PR) containing public key in credential and mixed after I will add --pub to publickey option (now I get --xpub) cardano-addresses very soon.

Plus I have problem with this trimming - cannot revert it

ApiScript correct stack.yaml stack.yaml Regenerate nix better Malformed

refine types make unit tests pass finally

add AnyAddress to swagger and and core unit tests fixup! Regenerate nix

…there plus code reshuffling

fix malformed and change heap size regenerate ApiAddressData some swagger fixes and reuse of Script Arbitrary instance from cardano-addresses hlint

…ub key

git checkout master -- stack.yaml git reset -p (use that to unstage the first hunk) git commit git checkout -- stack.yaml

final polishing Clean up Api.Types aeson instances 201 -> 202 status code plus staking -> stake, spending -> payment in ApiAddressData HLint

paweljakubas · 2020-11-05T18:31:18Z

bors r+

iohk-bors · 2020-11-05T19:57:15Z

Build succeeded:

paweljakubas self-assigned this Oct 16, 2020

paweljakubas requested a review from KtorZ October 16, 2020 11:11

paweljakubas commented Oct 16, 2020

View reviewed changes

KtorZ reviewed Oct 16, 2020

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-494/post-script-address branch from ccd4eb4 to 2f91803 Compare October 22, 2020 11:22

paweljakubas changed the title ~~add swagger with script as string~~ add POST endpoint for script address Oct 22, 2020

paweljakubas force-pushed the paweljakubas/adp-494/post-script-address branch from 12777ed to a3ed826 Compare October 22, 2020 14:23

KtorZ reviewed Oct 22, 2020

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-494/post-script-address branch 5 times, most recently from 07e4047 to 6b90e68 Compare October 26, 2020 18:06

paweljakubas commented Oct 27, 2020

View reviewed changes

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

paweljakubas marked this pull request as ready for review October 27, 2020 14:16

paweljakubas requested review from rvl and KtorZ October 27, 2020 14:16

KtorZ reviewed Oct 27, 2020

View reviewed changes

rvl reviewed Oct 28, 2020

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-494/post-script-address branch from 2123387 to 1cdaf25 Compare October 28, 2020 18:29

paweljakubas changed the title ~~add POST endpoint for script address~~ ADP-494 - Add POST endpoint for script address Oct 28, 2020

paweljakubas force-pushed the paweljakubas/adp-494/post-script-address branch 2 times, most recently from e6c0510 to 6459319 Compare October 30, 2020 12:13

piotr-iohk reviewed Oct 30, 2020

View reviewed changes

paweljakubas and others added 25 commits November 5, 2020 19:25

add endpoint

eee6f17

introduce ApiScript

5c9ad40

update core:unit

74fe268

ApiScript correct stack.yaml stack.yaml Regenerate nix better Malformed

add ApiCredential and ApiCredentials

9319cb1

refine types make unit tests pass finally

add AnyAddress

60066ec

add AnyAddress to swagger and and core unit tests fixup! Regenerate nix

implementation of enterprise addresses

03524d0

add reward account impl

c456474

add base address impl

82f083a

add Api.Link for postAnyAddress

5322b95

integration tests showing script enterprise addresses work

5bb1383

renaming to ApiCredential and change data constructors

41a1e1c

move postAnyAddress to shelley and use proper network discrimination …

6d95b74

…there plus code reshuffling

ApiCredentials -> ApiAddressData

cd79af0

fix malformed and change heap size regenerate ApiAddressData some swagger fixes and reuse of Script Arbitrary instance from cardano-addresses hlint

add golden tests for enterprise address with script

8c63aa9

add golden for reward account and base addresses with script

6831d1e

more precise swagger examples and shelley restore compilation fix

c6ee9ba

enterprise from pub key golden test

043b1f0

add reward account from public key golden test

5f0be33

Delegation address with both pub key credentials - golden tests

b51857e

delegating address golden test - spending from script, staking from p…

d8b140a

…ub key

delegating address golden test - spending from key, staking from script

ce151c6

Reset auto-generated whitespace in stack.yaml

4f14272

git checkout master -- stack.yaml git reset -p (use that to unstage the first hunk) git commit git checkout -- stack.yaml

Update CLI golden test

18b825a

final polishing Clean up Api.Types aeson instances 201 -> 202 status code plus staking -> stake, spending -> payment in ApiAddressData HLint

drop script and pub_key

c50b1c5

Regenerate nix

cf40f51

paweljakubas force-pushed the paweljakubas/adp-494/post-script-address branch from 435400b to cf40f51 Compare November 5, 2020 18:26

iohk-bors bot merged commit 20cebfe into master Nov 5, 2020

iohk-bors bot deleted the paweljakubas/adp-494/post-script-address branch November 5, 2020 19:57

ADP-494 - Add POST endpoint for script address #2253

ADP-494 - Add POST endpoint for script address #2253

Conversation

paweljakubas commented Oct 16, 2020 • edited by rvl Loading

Issue Number

Overview

Comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Oct 30, 2020 • edited Loading

paweljakubas commented Nov 5, 2020

iohk-bors bot commented Nov 5, 2020

paweljakubas commented Oct 16, 2020 •

edited by rvl

Loading

KtorZ Oct 16, 2020 •

edited

Loading

KtorZ Oct 16, 2020 •

edited

Loading

paweljakubas Oct 16, 2020 •

edited

Loading

paweljakubas Oct 23, 2020 •

edited

Loading

paweljakubas commented Oct 30, 2020 •

edited

Loading