refactor: improve string type safety #145

mkazlauskas · 2021-11-23T17:44:01Z

Context

Currently all string types are simple aliases of string. We have a lot of different types of strings for keys and hashes and we should utilize TypeScript to ensure that the string we get is the correct type that is not ambiguous.

Proposed Solution

Convert all string type aliases to more restricted types and validate them when creating. (BREAKING)

Important Changes Introduced

Add dependency to bech32 package
Add ProviderFailure.InvalidResponse
Rename certificate fields from address to rewardAccount where appropriate (BREAKING)
Convert transaction witness, Assets and TokenMap to Map (BREAKING)

Follow-up work

Address type doesn't accept base58-encoded Byron addresses and Address.util.isAddress does consider it valid. We need to decide whether we want to support it. If we do, there will be more code changes with CSL usage.
Might be a good idea to write more e2e tests with Blockfrost to ensure hash types are aligned. If we do that, we should dig up testnet transactions with certificates. What is the easiest way to do that?
There might still some type aliases from Ogmios used implicitly via nested types. I did not check every single type imported from Ogmios, but this PR converted the important string type aliases that we use and I suggest to convert the remainder (if there are any) as we go.

packages/core/src/Cardano/types/Strings/util.ts

packages/core/test/Cardano/types/Strings/util.test.ts

rhyslbw

I'm wondering if we'll have a cleaner model by removing the types/Strings/ path withing src/Cardano, and just have say src/Cardano/StakePool.ts, since this module contains more than type definitions, and using the primitive type to categorise is a bit redundant for a domain layer. Not a blocker, but just wanted to mention it before proceeding with the others.

mkazlauskas · 2021-11-24T07:40:27Z

I'm wondering if we'll have a cleaner model by removing the types/Strings/ path withing src/Cardano, and just have say src/Cardano/StakePool.ts, since this module contains more than type definitions, and using the primitive type to categorise is a bit redundant for a domain layer. Not a blocker, but just wanted to mention it before proceeding with the others.

Sure, let's do that

packages/golden-test-generator/src/AddressBalance/getOnChainAddressBalances.ts

packages/core/src/Cardano/types/AuxiliaryData.ts

packages/core/src/Cardano/types/Certificate.ts

…VrfKeyHash types

…nge type

sadasant · 2021-11-26T14:04:07Z

packages/core/src/Cardano/types/Transaction.ts

+  // TODO: need to find an example of this to verify type and length
+  // This is the type we use for witness object keys too, which kinda
+  // makes sense to have as bech32, because you might want to display
+  // it to the user to show who signed the tx


(Hello! I’m new to Cardano. I want to start contributing in one way or another! The future of global finances etc etc)

What about making an issue instead of a TODO comment? The issue could be linked to this PR (as a comment on this line) and anybody looking to collaborate could try to pick it up later.

I also think that bech32 sounds good for the reasons stated in this comment, so I wonder: why not just making it bech32?

Hi @sadasant, thanks for the contribution 👍

We find annotating the code keeps it fresh in mind during this early development phase, with the todos reflected in our internal project management system. We're currently reserving Github issues for external submissions, but we'll move to a more traditional OS model with contributor guidelines once things stabilise. As it stands, code contributions would be considered, but we're just not actively seeking them right now.

Hi @sadasant, thank you for a review! It is currently validated as bech32 with ed25519_pk prefix.

@rhyslbw some extra context for this TODO: Blockfrost doesn't seem to support requiredExtraSignatures field and I don't see it in cardano-graphql docs. This field is from Ogmios types and it's a Hash16 in there.

This Ogmios test vector has b5ae663aaea8e500157bdf4baafd6f5ba0ce5759f7cd4101fc132f54 (56 chars):

https://raw.githubusercontent.com/CardanoSolutions/ogmios/master/server/test/vectors/ChainSync/Response/RequestNext/135.json

Would you prefer our type to be hex as in ogmios instead of bech32?

Sorry, no as you were. In general we should be using the bech32 version if a prefix has been defined in the CIP

Prefix ed25519_pk that it currently uses is not listed in cip5 and CSL's PublicKey.to_bech32() does not accept a prefix argument. I think what we want is addr_vk and stake_vk prefixes. I already added a dependency to bech32 npm package so we can create the appropriate string from PublicKey.as_bytes()

(Thank you for your answers, @rhyslbw and @mkazlauskas 🙏)

sadasant · 2021-11-26T14:12:41Z

This PR looks really good! Makes the project way more robust in multiple fronts. I also appreciate the tests!

packages/blockfrost/src/blockfrostWalletProvider.ts

packages/blockfrost/test/blockfrostWalletProvider.test.ts

packages/core/src/Cardano/types/AuxiliaryData.ts

packages/core/src/Cardano/types/Certificate.ts

packages/core/src/Cardano/types/StakePool/StakePool.ts

rhyslbw · 2021-11-29T08:51:18Z

packages/core/src/Cardano/types/Transaction.ts

+  // TODO: need to find an example of this to verify type and length
+  // This is the type we use for witness object keys too, which kinda
+  // makes sense to have as bech32, because you might want to display
+  // it to the user to show who signed the tx


This Ogmios test vector has b5ae663aaea8e500157bdf4baafd6f5ba0ce5759f7cd4101fc132f54 (56 chars):

https://raw.githubusercontent.com/CardanoSolutions/ogmios/master/server/test/vectors/ChainSync/Response/RequestNext/135.json

packages/core/test/Cardano/types/Block.test.ts

packages/core/src/Cardano/types/StakePool/StakePool.ts

rhyslbw · 2021-11-29T14:09:17Z

Address type doesn't accept base58-encoded Byron addresses and Address.util.isAddress does consider it valid. We need to decide whether we want to support it. If we do, there will be more code changes with CSL usage.

All wallets need capability to send to legacy addresses, as they are still considered valid addresses just with limited utility.

rhyslbw

Awesome work here @mkazlauskas 🎉

Please note the conventional commits standard for representing breaking changes:

The reason I've not rejected the commits here is that we're not maintaining a stable API yet and pre-production minor versioning is not based on breaking changes, but moving forward it will be good to get into the habit so these details are second nature post-1.0. standard-version interprets this to decide the next version, so it's imperative for automating the release process, in addition to the benefits the standard brings to developers.

rhyslbw · 2021-11-30T03:04:04Z

packages/core/test/Cardano/types/Asset.test.ts

@@ -1,16 +1,57 @@
-import { AssetFingerprint, AssetId, AssetName, PolicyId } from '../../../src/Cardano';
+import { AssetFingerprint, AssetId, AssetName, PolicyId, util } from '../../../src/Cardano';


Nice tests, very expressive.

mkazlauskas requested a review from rhyslbw November 23, 2021 17:44

rhyslbw reviewed Nov 24, 2021

View reviewed changes

packages/core/src/Cardano/types/Strings/util.ts Outdated Show resolved Hide resolved

packages/core/test/Cardano/types/Strings/util.test.ts Outdated Show resolved Hide resolved

packages/core/test/Cardano/types/Strings/util.test.ts Outdated Show resolved Hide resolved

rhyslbw requested changes Nov 24, 2021

View reviewed changes

packages/core/test/Cardano/types/Strings/util.test.ts Outdated Show resolved Hide resolved

mkazlauskas force-pushed the refactor/type-safety branch from 0849fc1 to a325968 Compare November 24, 2021 07:25

rhyslbw self-requested a review November 24, 2021 07:31

rhyslbw reviewed Nov 24, 2021

View reviewed changes

mkazlauskas force-pushed the refactor/type-safety branch from a325968 to 63af887 Compare November 24, 2021 08:17

mkazlauskas requested a review from rhyslbw November 24, 2021 08:17

rhyslbw previously approved these changes Nov 24, 2021

View reviewed changes

mkazlauskas dismissed rhyslbw’s stale review via 11abd49 November 24, 2021 16:19

mkazlauskas force-pushed the refactor/type-safety branch from 11abd49 to b2f5d77 Compare November 24, 2021 16:36

mkazlauskas added 4 commits November 24, 2021 19:12

feat(core): add utils for custom string types, safer pool id types

bd430f6

refactor: add ProviderFailure.InvalidResponse, use improved PoolId type

91c0f59

refactor(core): convert Cardano/util and Cardano/StakePool into dirs

bd6506d

refactor: improve Cardano.Address and Cardano.RewardAccount types

eeb2074

mkazlauskas force-pushed the refactor/type-safety branch 2 times, most recently from 10c34ec to d326a17 Compare November 24, 2021 17:22

mkazlauskas added 4 commits November 24, 2021 19:34

refactor: improve Cardano.TransactionId type

90a1544

refactor: add Cardano.BlockId type

b24a899

refactor: add Ed25519Signature and Ed25519PublicKey types

bc8953d

refactor: update StakePool.rewardAccount to Cardano.RewardAccount

9ee9070

mkazlauskas force-pushed the refactor/type-safety branch from d326a17 to 9ee9070 Compare November 24, 2021 17:34

rhyslbw reviewed Nov 25, 2021

View reviewed changes

packages/golden-test-generator/src/AddressBalance/getOnChainAddressBalances.ts Outdated Show resolved Hide resolved

packages/core/src/Cardano/types/AuxiliaryData.ts Outdated Show resolved Hide resolved

packages/core/src/Cardano/types/Certificate.ts Outdated Show resolved Hide resolved

mkazlauskas added 5 commits November 25, 2021 12:28

refactor: convert Hash16 to a validated type, add Ed25519KeyHash and …

98ce920

…VrfKeyHash types

refactor: change StakePool.owners to be Cardano.RewardAccount[]

3f3dad6

refactor: split Hash16 into 2 more concrete types

b2c3048

refactor: convert TxAlonzo.witness.signatures to Map, resolve some TODOs

28de368

refactor: rename MIR certificate 'address' to 'rewardAccount' and cha…

28f2ccf

…nge type

mkazlauskas marked this pull request as ready for review November 25, 2021 14:47

mkazlauskas requested a review from rhyslbw November 25, 2021 14:48

mkazlauskas added 2 commits November 26, 2021 09:14

refactor: improve Tip type to use the new BlockId type

4fc6ad8

refactor: improve asset id types, convert TokenMap to Map

9781ae2

mkazlauskas force-pushed the refactor/type-safety branch from c020610 to 9781ae2 Compare November 26, 2021 11:57

sadasant reviewed Nov 26, 2021

View reviewed changes

rhyslbw requested changes Nov 29, 2021

View reviewed changes

mkazlauskas added 4 commits November 29, 2021 12:19

fix: change stakepool metadata extVkey field type to bech32 string

ec523a7

chore: rm some obsolete TODO comments

69e1f8e

refactor: convert tx signature key type to hex

854e9cb

refactor: change PoolParameters.vrf type to VrkVkHex

d4200ef

mkazlauskas requested a review from rhyslbw November 29, 2021 13:40

refactor(core): change type of requiredExtraSignatures to Ed25519KeyHash

914fab8

rhyslbw requested changes Nov 29, 2021

View reviewed changes

packages/core/test/Cardano/types/Block.test.ts Outdated Show resolved Hide resolved

packages/core/src/Cardano/types/StakePool/StakePool.ts Outdated Show resolved Hide resolved

refactor: improve name for PoolMdVk, improve opaque string tests

4ab2fda

rhyslbw approved these changes Nov 30, 2021

View reviewed changes

rhyslbw merged commit 365e7a6 into master Nov 30, 2021

rhyslbw deleted the refactor/type-safety branch November 30, 2021 03:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: improve string type safety #145

refactor: improve string type safety #145

mkazlauskas commented Nov 23, 2021 •

edited

Loading

rhyslbw left a comment •

edited

Loading

mkazlauskas commented Nov 24, 2021

sadasant Nov 26, 2021

rhyslbw Nov 29, 2021

mkazlauskas Nov 29, 2021

rhyslbw Nov 29, 2021

mkazlauskas Nov 29, 2021

rhyslbw Nov 29, 2021

mkazlauskas Nov 29, 2021 •

edited

Loading

sadasant Nov 29, 2021

sadasant commented Nov 26, 2021

rhyslbw Nov 29, 2021

rhyslbw commented Nov 29, 2021

rhyslbw left a comment

rhyslbw Nov 30, 2021

		@@ -1,16 +1,57 @@
		import { AssetFingerprint, AssetId, AssetName, PolicyId } from '../../../src/Cardano';
		import { AssetFingerprint, AssetId, AssetName, PolicyId, util } from '../../../src/Cardano';

refactor: improve string type safety #145

refactor: improve string type safety #145

Conversation

mkazlauskas commented Nov 23, 2021 • edited Loading

Context

Proposed Solution

Important Changes Introduced

Follow-up work

rhyslbw left a comment • edited Loading

Choose a reason for hiding this comment

mkazlauskas commented Nov 24, 2021

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

mkazlauskas Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant commented Nov 26, 2021

Choose a reason for hiding this comment

rhyslbw commented Nov 29, 2021

rhyslbw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkazlauskas commented Nov 23, 2021 •

edited

Loading

rhyslbw left a comment •

edited

Loading

mkazlauskas Nov 29, 2021 •

edited

Loading