Enable strict null checks #1192

coreyar · 2021-10-26T16:24:25Z

This PR enables strict null checks in the typescript config.

Many changes were straightforward like explicitly declaring null or undefined types or enforcing a final type from logical operators using typecasting or ternaries.

Less straightforward changes include handling a missing wallet (it could be set to null when logged out). In these cases, I wrapped logic required the existence of a value in a conditional statement. I also added a function for type narrowing to check if the transaction plan result is a success or error and added an error case for if the transaction plan wasn't executed because of a missing wallet.

refi93 · 2021-11-02T12:11:35Z

app/frontend/wallet/shelley/types.ts

-  auxiliaryDataHash: HexString
-  validityIntervalStart: number
+  auxiliaryData: TxAuxiliaryData | null
+  auxiliaryDataHash: HexString | undefined


I would rather keep this as HexString | null and modify the upstream code to pass such values instead of undefined, so it's clear where such potentially undefined values enter our logic and that we handle them properly, reducing the risk of bugs due to accidentally omitting some parameter only for it to end up as undefined without us noticing

refi93 · 2021-11-02T12:48:31Z

app/frontend/actions/common.ts

    try {
-      return await getWallet()
+      if (!wallet) {
+        throw new Error('Wallet is not loaded')


I would rather extract this apparently repeated logic into a getWalletOrThrow() helper function which would be guaranteed to return the wallet object, making the places where we invoke it less cluttered with checks that shouldn't really trigger at runtime unless there's a bug in which case we want to notice it asap

refi93 · 2021-11-02T12:50:25Z

app/frontend/actions/accounts.ts

@@ -54,7 +60,7 @@ export default (store: Store) => {
      targetAccountIndex: accountIndex,
    })
    const targetAddress = await getWallet()
-      .getAccount(accountIndex)
+      ?.getAccount(accountIndex)


the wallet object shouldn't be undefined/null here, otherwise it could lead to inonsistent behavior down the line. The original code would just fail so I wouldn't be worried to throw instead of trying to handle it "gracefully".

As suggested in another comment, I'd rather introduce a getWalletOrThrow() method which would assert that the wallet is properly loaded, de-cluttering the downstream logic from having to handle a case that shouldn't happen unless there's a bug in which case getting an error as early as possible seems beneficial

refi93 · 2021-11-02T13:28:57Z

app/frontend/actions/delegate.ts

-    const poolInfo = !state.shelleyDelegation?.selectedPool?.name
-      ? await getWallet().getPoolInfo(state.shelleyDelegation?.selectedPool?.url)
-      : {}
+    const poolInfo = (!state.shelleyDelegation?.selectedPool?.name &&


This overriding of potentially undefined values with empty ones seems fragile and I'd have a hard time reasoning about the possible consequences of it. I think it can be rewritten more sensibly like this:

const selectedPool = state.shelleyDelegation?.selectedPool const poolInfo = (!selectedPool?.name && selectedPool?.url) ? (await getWallet()?.getPoolInfo(selectedPool?.url)) : null if (hasPoolIdentifiersChanged(state)) { return } const newState = getState() setState({ shelleyDelegation: { ...state.shelleyDelegation, selectedPool: selectedPool ? { ...selectedPool, ...poolInfo, } : null,

@PeterBenc please check if I'm not missing anything by any chance as I think you were originally working on this

Yeah I agree, we shouldn't overide the values, that handled in stakePoolInfo.tsx file, What @refi93 proposes should imho work as expected so I am curious what was the reason to change the previous behavior, the difference being only doing ...null instead of ...{} which should be the same.

The problem was that type for Stakepool doesn't have any optional properties. As it was written if the poolInfo or selectedPool can't be fetched or doesn't exist then you end up with an empty object. I was thinking it would be better to have defaults than to type all the properties as optional.

I am updating with the solution proposed by @refi93 because it does make more sense

refi93 · 2021-11-02T13:32:41Z

Thanks a lot @coreyar for your PR and sorry for the late feedback, we were quite busy last week. I left a few comments/suggestions which I believe should simplify the solution

refi93 · 2021-11-02T13:58:01Z

app/tests/src/transaction-planner.js

@@ -106,7 +108,7 @@ describe('Succesful transaction plans', () => {
    it(`should ${name}`, () => {
      const {utxos, changeAddress, fee, args} = setting
      const txPlanResult = selectMinimalTxPlan(utxos, changeAddress, args)
-      if (txPlanResult.success === true) {
+      if (isTxPlanResultSuccess(txPlanResult)) {


I would rather replace the one-line helper with its explicit implementation as it doesn't really simplify the logic, but it arguably adds one more layer of indirection to whoever wants to understand what this check does. Also it seems a bit weird for a function that seems to expect a tx plan to also allow null/undefined and hide it in its signature, possibly misleading someone casually checking this code that the tx plan must be defined

refi93 · 2021-11-02T14:01:31Z

app/frontend/wallet/shelley/shelley-trezor-crypto-provider.ts

@@ -401,11 +404,11 @@ const ShelleyTrezorCryptoProvider = async ({
        return {
          finalizedTxAux: ShelleyTxAux({
            ...txAux,
-            auxiliaryDataHash: auxiliaryDataSupplement.auxiliaryDataHash,
+            auxiliaryDataHash: auxiliaryDataSupplement?.auxiliaryDataHash,


auxiliaryDataSupplement shouldn't really be undefined here, by the contract of the function called it should exist (check docs https://github.com/trezor/connect/blob/develop/docs/methods/cardanoSignTransaction.md#catalyst-voting-key-registration), so I'd rather add here a runtime assertion on auxiliaryDataSupplement being defined (!= null) and pass auxiliaryDataSupplement.auxiliaryDataHash

I am seeing in the docs that it can be undefined https://github.com/trezor/connect/blob/2955b5ef92a3c77ccf3b1353f74f37fa4a2ff337/docs/methods/cardanoSignTransaction.md#flowtype-1

I can still replace it with a runtime assertion

yes, indeed the docs aren't very clear on that now that I checked it deeper, sorry for the confusion, but it's supposed to be defined anyway for catalyst registrations and if not, this code wouldn't work as we need these data in such case. So a runtime assertion makes sense

refi93 · 2021-11-02T14:01:56Z

app/frontend/wallet/shelley/shelley-ledger-crypto-provider.ts

          }),
          txAuxiliaryData: cborizeTxAuxiliaryVotingData(
            txAux.auxiliaryData,
-            auxiliaryDataSupplement.catalystRegistrationSignatureHex
+            auxiliaryDataSupplement?.catalystRegistrationSignatureHex


same as in trezor crypto provider, related ledgerjs docs: https://vacuumlabs.github.io/ledgerjs-cardano-shelley/4.0.0/index.html#txauxiliarydata

refi93 · 2021-11-02T14:03:40Z

app/frontend/wallet/shelley/shelley-transaction.ts

@@ -345,11 +345,11 @@ const cborizeTxVotingRegistration = ({

 const cborizeTxAuxiliaryVotingData = (
  txAuxiliaryData: TxAuxiliaryData,
-  signatureHex: string
+  signatureHex: string | undefined


I see in the upstream logic you relaxed the type of the signatureHex parameter, but it actually should always be defined

I allowed for this to be undefined because it is called with auxiliaryDataSupplement.catalystSignature as the value for signatureHex which can be undefined

https://github.com/trezor/connect/blob/2955b5ef92a3c77ccf3b1353f74f37fa4a2ff337/src/ts/types/networks/cardano.d.ts#L201

I can keep this type as defined and add a runtime check for where it is called with the undefined value

as explained in #1192 (comment) let's add a runtime check 👍

refi93 · 2021-11-02T14:24:24Z

app/frontend/wallet/shelley/shelley-ledger-crypto-provider.ts

-const getLedgerTransport = async (ledgerTransportType: LedgerTransportType): Promise<Transport> => {
+const getLedgerTransport = async (
+  ledgerTransportType: LedgerTransportType
+): Promise<Transport | null> => {


this code should not deal with any other ledger transport type than the ones handled here (webhid, u2f, webusb) and should throw in such case, so it should either throw or return a proper Transport. The "default" option matters really just for the UI. I would propose refactoring it, even within this PR as it shouldn't be much work and would be less confusing than relaxing the type signature of this function. Here is how I' approach it:

Introduce a LedgerTransportChoice type which would be equivalent to the current LedgerTransportType and rename all existing instances in the code to LedgerTransportChoice

introduce type LedgerTransportType = Exclude<LedgerTransportChoice, LedgerTransportChoice.DEFAULT> and migrate from LedgerTransportChoice to LedgerTransportType in getLedgerTransport() output type and getDefaultLedgerTransportType() function and anything that is impacted by that

getLedgerTransport() now can be refactored to return assertUnreachable in the default branch of the switch statement

coreyar · 2021-11-02T16:11:32Z

@refi93 Thank you for the feedback. I have updated the PR with your feedback and left comments where clarification was needed.

refi93 · 2021-11-02T16:41:36Z

app/frontend/actions/reloadWallet.ts

-      const accountsInfo = await wallet.getAccountsInfo(state.validStakepoolDataProvider)
-      const tokensMetadata = await wallet.getTokensMetadata(accountsInfo)
+      const accountsInfo = state.validStakepoolDataProvider
+        ? await wallet?.getAccountsInfo(state.validStakepoolDataProvider)


getWalletOrThrow() should ensure that the wallet is defined

Suggested change

? await wallet?.getAccountsInfo(state.validStakepoolDataProvider)

? await wallet.getAccountsInfo(state.validStakepoolDataProvider)

refi93 · 2021-11-02T16:46:53Z

app/frontend/actions/wallet.ts


+export const getWalletOrThrow = (): Wallet => {


I see that getWallet() is not used anymore anywhere else (sorry I originally thought we legitimately check somewhere whether the wallet is loaded or not, but apparently not), so I'd rather rename getWalletOrThrow() to getWallet() and get rid of the old getWallet() function that wasn't checking whether the wallet is ready as the OrThrow suffix is arguably just exposing a lower-level concern than the callers would care about most of the time

refi93 · 2021-11-02T16:49:38Z

app/frontend/actions/send.ts

-      const tokenBalance = getSourceAccountInfo(state).tokenBalance.find(
-        (token) => token.policyId === policyId && token.assetName === assetName
-      ).quantity
+      const tokenBalance = getSourceAccountInfo(state)?.tokenBalance?.find(


getSourceAccountInfo() should throw if there's no source account, the original code would fail anyway if the source account isn't defined, so I'd add the assertion right in the declaration of getSourceAccountInfo() rather than relaxing the type of tokenBalance and adding an arguably redundant fallback to zero below

app/frontend/actions/delegate.ts

refi93 · 2021-11-02T17:01:06Z

app/frontend/components/pages/accounts/accountTile.tsx

@@ -12,7 +12,7 @@ type TileProps = {
  accountIndex: number
  ticker: string | null
  availableBalance: number | null
-  rewardsBalance: number | null
+  rewardsBalance: number | null | undefined


The AccountInfo.shelleyBalances type can be updated to have its properties non-optional as the places where it's set is set to a number always anyway, making this type broadening to accept undefined redunant

refi93 · 2021-11-02T17:02:06Z

app/frontend/components/common/viewPort.tsx

-const isBiggerThanMobile = (screenType: ScreenType): boolean => {
-  return [ScreenType.TABLET, ScreenType.DESKTOP].includes(screenType)
+const isBiggerThanMobile = (screenType: ScreenType | undefined): boolean => {
+  return !!(screenType && [ScreenType.TABLET, ScreenType.DESKTOP].includes(screenType))


I'd rather keep the interface of this function oblivious to the fact screenType might be undefined and move the concern of handling that to the caller as they should have the context to determine if that's a valid case or not and handle that more cleanly

This could also be handled by setting a default value for the state managed by the useViewport hook. Which would be similar to setting a fallback when the caller checks if the value is undefined or not.

refi93 · 2021-11-02T17:12:07Z

app/frontend/actions/delegate.ts

-          ...newState.shelleyDelegation,
-          delegationFee: (txPlanResult.txPlan.fee + txPlanResult.txPlan.deposit) as Lovelace,
-        },
+    if (stakingAddress && poolHash) {


rather than nesting the logic under this condition, I'd put here a runtime assertion that stakingAddress must not be null as the original logic apparently relied on that and I don't see it working properly without that assumption being fulfilled and it doesn't really make sense to delegate without the staking address being set

Since this logic will be repeated in four places does it make sense to throw this error in prepareTxPlan and relax the stakingAddress type in the signature?

I looked deeper into that and the only place where we set the stakingKey to null is the initial state of the app where we set an initial value to the accountsInfo property, but that doesn't really make sense. Let's just set accountsInfo there to an empty array and then we should be good to enforce that stakingAddress must be always defined

I mean here:

adalite/app/frontend/state.ts

Line 223 in 3119857

accountsInfo: [

Do you see some reason @PeterBenc why we would have to set some default "null" account in the initial state of the app, or is that actually just legacy from the times when we had a single account in the state?

Agreed, I dont see any reason why accountInfos cant be initially []. Its not used until the wallet is loaded and then just assigned.

BUT, I think I remember that setting account info to [] was actually a problem when logging out. I think that after logging out, some components still (for a brief moment I guess) "used" accountInfos[state.activeAccount] and accessed its keys so errors like cant... something of undefined popped up and the page crashed. Surely that probably points to an issue on its own.

So that was probably the reason for keeping it the way it is now, but I just tried setting it to [] and it works as expected. So I guess I either remember it wrong the issue causing it got fixed.

Anyway, just something to consider but a bit unrelated to this, having the initial accountsInfo as [] seem to work and makes sense.

refi93 · 2021-11-02T17:13:26Z

app/frontend/actions/delegate.ts

-    })
-    if (txPlanResult.success === true) {
+    let txPlanResult: TxPlanResult | null | undefined = null
+    if (sourceAccount.stakingAddress) {


same as https://github.com/vacuumlabs/adalite/pull/1192/files#r741304487

refi93 · 2021-11-02T17:13:48Z

app/frontend/actions/transaction.ts

    // TODO: rewards should be of type Lovelace
    const rewards = getSourceAccountInfo(state).shelleyBalances.rewardsAccountBalance as Lovelace
    const stakingAddress = getSourceAccountInfo(state).stakingAddress
-    const txPlanResult = await prepareTxPlan({rewards, stakingAddress, txType: TxType.WITHDRAW})
+    if (stakingAddress) {


same as https://github.com/vacuumlabs/adalite/pull/1192/files#r741304487

refi93 · 2021-11-02T17:14:01Z

app/frontend/actions/voting.tsx

-      nonce: BigInt(nonce),
-    })
+    let txPlanResult: TxPlanResult | null | undefined = null
+    if (sourceAccount.stakingAddress) {


same as https://github.com/vacuumlabs/adalite/pull/1192/files#r741304487

refi93 · 2021-11-02T17:17:06Z

app/frontend/wallet/shelley/shelley-ledger-crypto-provider.ts

@@ -338,7 +339,7 @@ const ShelleyLedgerCryptoProvider = async ({
    const poolOwners: LedgerTypes.PoolOwner[] = stakepoolOwners.map((owner) => {
      // TODO: helper function for slicing first bit from staking address so its stakingKeyHash
      return !Buffer.compare(


Overriding the value to an empty string seems really fragile. I checked the TxStakepoolOwner type and it doesn't really make sense for it to have one optional property and nothing else, and changing it to :

export type TxStakepoolOwner = { stakingKeyHashHex: string }

worked, removing the need to handle its undefinedness. The parseStakepoolOwners function where these owners come from apparently always returns them, so the original TxStakepoolOwner type was apparently just too broad, probably as legacy from some earlier state when there were multiple optional properties in the object

refi93 · 2021-11-02T17:24:45Z

app/frontend/wallet/shelley/shelley-ledger-crypto-provider.ts

@@ -347,7 +348,7 @@ const ShelleyLedgerCryptoProvider = async ({
        }
        : {
          type: LedgerTypes.PoolOwnerType.THIRD_PARTY,
-          params: {stakingKeyHashHex: owner.stakingKeyHashHex},
+          params: {stakingKeyHashHex: owner.stakingKeyHashHex || ''},


same as https://github.com/vacuumlabs/adalite/pull/1192/files#r741308487

refi93 · 2021-11-02T17:24:56Z

app/frontend/wallet/shelley/shelley-transaction.ts

@@ -212,7 +212,7 @@ function cborizeStakepoolRegistrationCert(
    ),
    Buffer.from(poolRegistrationParams.rewardAccountHex, 'hex'),
    poolRegistrationParams.poolOwners.map((ownerObj) => {
-      return Buffer.from(ownerObj.stakingKeyHashHex, 'hex')
+      return Buffer.from(ownerObj.stakingKeyHashHex || '', 'hex')


same as https://github.com/vacuumlabs/adalite/pull/1192/files#r741308487

refi93 · 2021-11-02T17:25:48Z

app/frontend/wallet/shelley/shelley-transaction.ts

@@ -349,7 +349,7 @@ const cborizeTxAuxiliaryVotingData = (
 ): CborizedVotingRegistrationMetadata => [
  new Map<number, Map<number, Buffer | BigInt>>([
    cborizeTxVotingRegistration(txAuxiliaryData),
-    [61285, new Map<number, Buffer>([[1, Buffer.from(signatureHex, 'hex')]])],
+    [61285, new Map<number, Buffer>([[1, Buffer.from(signatureHex || '', 'hex')]])],


this fallback to empty string seems very fragile and should no longer be needed

refi93 · 2021-11-02T17:27:20Z

app/frontend/wallet/shelley/shelley-trezor-crypto-provider.ts

-      ...(input.address && {path: addressToAbsPathMapper(input.address)}),
-      prev_hash: input.txHash,
+      path: input.address ? addressToAbsPathMapper(input.address) : [],
+      prev_hash: input.txHash || '',


this fallback to empty string shouldn't be needed

refi93 · 2021-11-02T17:28:32Z

app/frontend/wallet/shelley/shelley-trezor-crypto-provider.ts

@@ -332,7 +335,7 @@ const ShelleyTrezorCryptoProvider = async ({

  const prepareByronWitness = (witness: TrezorTypes.CardanoSignedTxWitness): TxByronWitness => {
    const publicKey = Buffer.from(witness.pubKey, 'hex')
-    const chainCode = Buffer.from(witness.chainCode, 'hex')
+    const chainCode = Buffer.from(witness.chainCode || '', 'hex')


byron witnesses should have a chain code, so this fallback to empty string is really fragile. A runtime asserttion would be more sensible

refi93 · 2021-11-02T17:29:36Z

app/frontend/wallet/shelley/shelley-trezor-crypto-provider.ts

@@ -373,10 +376,10 @@ const ShelleyTrezorCryptoProvider = async ({
        return {
          catalystRegistrationParameters: {
            votingPublicKey: txAuxiliaryData.votingPubKey,
-            stakingPath: txAuxiliaryData.rewardDestinationAddress.stakingPath,
+            stakingPath: txAuxiliaryData.rewardDestinationAddress.stakingPath || '',


rather than fallback to empty string which if triggered would break the code unpredictably, please put here a runtime assertion

refi93 · 2021-11-02T17:30:59Z

app/frontend/actions/voting.tsx

@@ -54,21 +56,25 @@ export default (store: Store) => {
    state = getState()

    const stakePubKey = xpub2pub(
-      Buffer.from(getSourceAccountInfo(state).stakingXpub.xpubHex, 'hex')
+      Buffer.from(getSourceAccountInfo(state).stakingXpub?.xpubHex || '', 'hex')


I'd rather put here a runtime assertion that this xpubHex exists - if it wouldn't, this code would break unpredictably

refi93 · 2021-11-02T17:31:30Z

app/frontend/actions/voting.tsx

+    const nonce =
+      (await getWalletOrThrow()
+        .getAccount(state.sourceAccountIndex)
+        .calculateTtl()) || ''


this fallback to empty string should no longer be needed

refi93 · 2021-11-02T17:35:51Z

app/frontend/wallet/blockchain-explorer.ts

    const tokenBundle = aggregateTokenBundles(addressTokenBundles).filter(
      (token) => token.quantity > 0
    )
    const coins = addressInfos.reduce(
-      (acc, elem) => acc + parseInt(elem.caBalance.getCoin, 10),
+      (acc, elem) => acc + parseInt(elem?.caBalance.getCoin || '', 10),


Suggested change

(acc, elem) => acc + parseInt(elem?.caBalance.getCoin || '', 10),

(acc, elem) => acc + parseInt(elem?.caBalance.getCoin || '0', 10),

refi93 · 2021-11-02T17:36:35Z

app/frontend/wallet/shelley/helpers/addresses.ts

@@ -39,8 +39,8 @@ export const encodeAssetFingerprint = (policyIdHex: HexString, assetNameHex: Hex
  return bech32.encode('asset', data)
 }

-export const encodeCatalystVotingKey = (votingKey: HexString): string => {
-  return bech32.encode('ed25519_pk', Buffer.from(votingKey, 'hex'))
+export const encodeCatalystVotingKey = (votingKey: HexString | null): string => {


I would rather make this function accept only a defined votingKey and let the upstream logic handle the case when it isn't (if it can actually happen at all)

refi93 · 2021-11-02T17:39:10Z

app/frontend/wallet/shelley/helpers/poolCertificateUtils.ts

          },
        }
      case TxRelayTypes.SINGLE_HOST_NAME:
        return {
          type: TxRelayType.SINGLE_HOST_NAME,
          params: {
            portNumber: relay.portNumber,
-            dnsName: relay.dnsName,
+            dnsName: relay.dnsName || '',


this fallback to empty string seems really fragile as it is apparently a required property as per the type signature. A runtime assertion on the property being defined would be more sensible, same below

refi93 · 2021-11-02T17:39:32Z

app/frontend/wallet/shelley/shelley-ledger-crypto-provider.ts

@@ -338,7 +339,7 @@ const ShelleyLedgerCryptoProvider = async ({
    const poolOwners: LedgerTypes.PoolOwner[] = stakepoolOwners.map((owner) => {
      // TODO: helper function for slicing first bit from staking address so its stakingKeyHash
      return !Buffer.compare(
-        Buffer.from(owner.stakingKeyHashHex, 'hex'),
+        Buffer.from(owner.stakingKeyHashHex || '', 'hex'),


same as https://github.com/vacuumlabs/adalite/pull/1192/files#r741308487

refi93 · 2021-11-02T17:39:57Z

app/frontend/wallet/shelley/shelley-trezor-crypto-provider.ts

@@ -236,7 +236,7 @@ const ShelleyTrezorCryptoProvider = async ({
    const {data} = bech32.decode(certificate.stakingAddress)
    const owners = certificate.poolRegistrationParams.poolOwners.map((owner) => {
      // TODO: helper function stakingAddress2StakingKeyHash
-      return !Buffer.compare(Buffer.from(owner.stakingKeyHashHex, 'hex'), data.slice(1))
+      return !Buffer.compare(Buffer.from(owner.stakingKeyHashHex || '', 'hex'), data.slice(1))


same as https://github.com/vacuumlabs/adalite/pull/1192/files#r741308487

refi93 · 2021-11-02T17:45:33Z

app/frontend/components/pages/sendAda/sendAdaPage.tsx

@@ -165,7 +167,7 @@ const SendAdaPage = ({

  const adaAsset: DropdownAssetItem = {
    type: AssetFamily.ADA,
-    policyId: null,
+    policyId: '',


non-null policyId doesn't make sense for ADA asse. The type of DropdownAssetItem should rather be fixed to have policyId as null for this case and as not-null for the case when the type is AssetFamily.TOKEN, leveraging possibly type union

refi93 · 2021-11-03T14:18:17Z

@coreyar I tried running the wallet with yarn dev and I got a wallet is not loaded error in the console and a white screen, so it seems that the assertion doesn't work everywhere it was put, here's the stacktrace:

Uncaught Error: Wallet is not loaded
    getWalletOrThrow wallet.ts:41
    default wallet.ts:54
    default reloadWallet.ts:10
    default transaction.ts:32
    default delegate.ts:24
    default accounts.ts:15
    default actions.ts:18
    mapActions util.js:9
    Preact 20
    reload walletApp.js:98
    js walletApp.js:105
    Webpack 6

coreyar · 2021-11-03T15:09:24Z

@coreyar I tried running the wallet with yarn dev and I got a wallet is not loaded error in the console and a white screen, so it seems that the assertion doesn't work everywhere it was put, here's the stacktrace:
Uncaught Error: Wallet is not loaded
    getWalletOrThrow wallet.ts:41
    default wallet.ts:54
    default reloadWallet.ts:10
    default transaction.ts:32
    default delegate.ts:24
    default accounts.ts:15
    default actions.ts:18
    mapActions util.js:9
    Preact 20
    reload walletApp.js:98
    js walletApp.js:105
    Webpack 6

Ok, I am not seeing any webpack errors building right now but let me investigate. I've pushed updates to the comments left so far

coreyar · 2021-11-03T15:50:52Z

@refi93 I found the error. The wallet was being loaded in an outer scope but used in the scope of inner functions. I solved this by fetching the wallet inside the functions where it is used so that it isn't prematurely loaded

Update: I'm testing to see how this update affects other parts of the application

coreyar · 2021-11-03T16:38:48Z

I left the null checks in loadWallet because if cryptoProvider isn't fetched then the wallet won't be created so it could be null. Another option would be to wrap all the subsequent logic in the cryptoProvider conditional block or wrap it in another block checking for the wallet.

It isn't absolutely necessary because it is wrapped in a try block with the catch setting an error that the wallet couldn't be loaded.

refi93 · 2021-11-03T17:00:38Z

app/frontend/actions/voting.tsx

@@ -52,23 +55,28 @@ export default (store: Store) => {
    }
    loadingAction(state, 'Preparing transaction...')
    state = getState()
-
+    const sourceAccountInfo = getSourceAccountInfo(state)
+    if (!sourceAccountInfo.stakingXpub?.xpubHex) {


for these runtime errors I would rather use a standard assert(condition) statement - these shouldn't really happen at runtime and should rather be "technical" assertions and are more concise than "if ... then ... throw".

We have an assert() function in our codebase which was used in tests so far but should be usable here as well

app/frontend/components/common/asset.tsx

refi93 · 2021-11-03T17:14:25Z

ok let's do it like this - this PR should already be close enough to what we want but needs further refinements and testing to tackle edge-cases that may not be as obvious and it would be more efficient for us to finish that, so I'd merge this to a enable-strict-null-checks branch in adalite repo and make he refinements there and then merge it to our develop branch

coreyar · 2021-11-03T17:23:51Z

@refi93 Sounds good. I've got time to pickup another issue if that would be helpful

refi93 · 2021-11-03T17:23:57Z

Thanks a lot @coreyar for your contribution, here's the follow up PR #1206 where we'd refactor it/fix it a bit more so it can be merged

refi93 · 2021-11-03T17:29:10Z

@coreyar great! - one issue that has been limiting us for a while is this one: #867 - currently the amounts are ordinary JS numbers which breaks as soon as you work with values bigger than max safe integer in JS which can happen especially with cardano tokens

Another, smaller issue that comes to my mind is this one: #1086

refi93 · 2021-11-03T17:36:51Z

The biginit migration would be pretty big, it probably makes sense to do so in several "layers", going from the innermost logic to the outermost one (or vice versa), if you choose to do it, so it can be split into multiple smaller PRs

refi93 · 2021-11-08T12:47:27Z

@coreyar another small issue worth doing can be this one: #1056

coreyar mentioned this pull request Oct 26, 2021

Enable typescript's strictNullChecks option #1001

Closed

refi93 reviewed Nov 2, 2021

View reviewed changes

DavidTranDucVL mentioned this pull request Nov 2, 2021

Display metadata token name instead of onchain asset name #1193

Merged

refi93 reviewed Nov 2, 2021

View reviewed changes

refi93 added the needs work label Nov 2, 2021

refi93 reviewed Nov 2, 2021

View reviewed changes

app/frontend/actions/delegate.ts Outdated Show resolved Hide resolved

refi93 reviewed Nov 2, 2021

View reviewed changes

refi93 reviewed Nov 3, 2021

View reviewed changes

app/frontend/components/common/asset.tsx Outdated Show resolved Hide resolved

refi93 changed the base branch from develop to enable-strict-null-checks November 3, 2021 17:17

refi93 approved these changes Nov 3, 2021

View reviewed changes

refi93 merged commit 93cbe13 into vacuumlabs:enable-strict-null-checks Nov 3, 2021

refi93 force-pushed the enable-strict-null-checks branch from 3119857 to 93cbe13 Compare November 3, 2021 17:21

refi93 mentioned this pull request Nov 3, 2021

Enable strict null checks #1206

Merged

mlenik temporarily deployed to adalite-enable-strict-n-yrkvce November 3, 2021 17:22 Inactive

coreyar deleted the enable-strict-null-checks branch November 3, 2021 17:24

	? await wallet?.getAccountsInfo(state.validStakepoolDataProvider)
	? await wallet.getAccountsInfo(state.validStakepoolDataProvider)

	(acc, elem) => acc + parseInt(elem?.caBalance.getCoin \|\| '', 10),
	(acc, elem) => acc + parseInt(elem?.caBalance.getCoin \|\| '0', 10),

Enable strict null checks #1192

Enable strict null checks #1192

Conversation

coreyar commented Oct 26, 2021

Choose a reason for hiding this comment

refi93 Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

refi93 Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

refi93 Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refi93 commented Nov 2, 2021 • edited Loading

refi93 Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

refi93 Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refi93 Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refi93 Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

refi93 Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

coreyar commented Nov 2, 2021

Choose a reason for hiding this comment

refi93 Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

refi93 Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refi93 Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refi93 Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

coreyar Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

refi93 Nov 3, 2021 • 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

refi93 Nov 2, 2021 • 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

Choose a reason for hiding this comment

refi93 commented Nov 3, 2021

coreyar commented Nov 3, 2021

coreyar commented Nov 3, 2021 • edited Loading

coreyar commented Nov 3, 2021

refi93 Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

refi93 commented Nov 3, 2021

coreyar commented Nov 3, 2021

refi93 commented Nov 3, 2021 • edited Loading

refi93 commented Nov 3, 2021

refi93 commented Nov 3, 2021

refi93 commented Nov 8, 2021

refi93 Nov 2, 2021 •

edited

Loading

refi93 Nov 2, 2021 •

edited

Loading

refi93 Nov 2, 2021 •

edited

Loading

refi93 commented Nov 2, 2021 •

edited

Loading

refi93 Nov 2, 2021 •

edited

Loading

refi93 Nov 2, 2021 •

edited

Loading

refi93 Nov 2, 2021 •

edited

Loading

refi93 Nov 2, 2021 •

edited

Loading

refi93 Nov 2, 2021 •

edited

Loading

refi93 Nov 2, 2021 •

edited

Loading

refi93 Nov 2, 2021 •

edited

Loading

refi93 Nov 2, 2021 •

edited

Loading

refi93 Nov 2, 2021 •

edited

Loading

coreyar Nov 3, 2021 •

edited

Loading

refi93 Nov 3, 2021 •

edited

Loading

refi93 Nov 2, 2021 •

edited

Loading

coreyar commented Nov 3, 2021 •

edited

Loading

refi93 Nov 3, 2021 •

edited

Loading

refi93 commented Nov 3, 2021 •

edited

Loading