Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Schema system refactor #10280

Merged
merged 11 commits into from
Jan 5, 2024
Merged

Conversation

martykan
Copy link
Member

@martykan martykan commented Dec 11, 2023

Effort to refactor the schema system based on my previous proposal

Description

  1. Created new package schema-utils, which is based on the Typebox library
  2. This package includes support for custom types (ArrayBuffer, Buffer, Uint), in a modular way. There are also patches for issues around enums and especially keyof enums.
  3. Added a codegen script which can convert existing types to our schemas. It is based on an existing Typebox solution, but with some changes for our usecases.
  4. Added update:schema command to the protobuf package, which generates message-schema.ts using the codegen script. This should be 1:1 equivalent with the existing types.
  5. Added an example showcasing possible integration in connect-explorer - added a page for authorizeCoinjoin, which is mapped from the schema
  6. Migrated validation in all API methods to the new schema system, except for methods that depend on blockchain-link (see below)

Potential breaking changes

The new schema system is more strict than the existing one. It is possible that some of the existing API methods will fail validation. This is a good thing, as it will prevent invalid calls to the device.
However it is not out of the question that the types may need to be adjusted to allow for valid calls.

Affected API methods

./api/applyFlags.ts
./api/applySettings.ts
./api/authenticateDevice.ts
./api/authorizeCoinjoin.ts
./api/binance/api/binanceGetAddress.ts
./api/binance/api/binanceGetPublicKey.ts
./api/binance/api/binanceSignTransaction.ts
./api/binance/binanceSignTx.ts
./api/bitcoin/refTx.ts
./api/blockchainDisconnect.ts
./api/cancelCoinjoinAuthorization.ts
./api/cardano/api/cardanoGetAddress.ts
./api/cardano/api/cardanoGetNativeScriptHash.ts
./api/cardano/api/cardanoGetPublicKey.ts
./api/cardano/api/cardanoSignTransaction.ts
./api/cardano/cardanoAddressParameters.ts
./api/cardano/cardanoAuxiliaryData.ts
./api/cardano/cardanoCertificate.ts
./api/cardano/cardanoInputs.ts
./api/cardano/cardanoOutputs.ts
./api/cardano/cardanoTokenBundle.ts
./api/changePin.ts
./api/cipherKeyValue.ts
./api/eos/api/eosGetPublicKey.ts
./api/eos/api/eosSignTransaction.ts
./api/ethereum/api/ethereumGetAddress.ts
./api/ethereum/api/ethereumGetPublicKey.ts
./api/ethereum/api/ethereumSignMessage.ts
./api/ethereum/api/ethereumSignTransaction.ts
./api/ethereum/api/ethereumSignTransaction.ts
./api/ethereum/api/ethereumSignTypedData.ts
./api/ethereum/api/ethereumVerifyMessage.ts
./api/ethereum/ethereumDefinitions.ts
./api/firmwareUpdate.ts
./api/getAccountDescriptor.ts
./api/getAddress.tsx
./api/getCoinInfo.ts
./api/getFirmwareHash.ts
./api/getOwnershipId.ts
./api/getOwnershipProof.ts
./api/getPublicKey.ts
./api/nem/api/nemGetAddress.ts
./api/nem/api/nemSignTransaction.ts
./api/pushTransaction.ts
./api/recoveryDevice.ts
./api/requestLogin.ts
./api/resetDevice.ts
./api/ripple/api/rippleGetAddress.ts
./api/ripple/api/rippleSignTransaction.ts
./api/setBusy.ts
./api/signMessage.ts
./api/solana/additionalInfo.ts
./api/solana/api/solanaGetAddress.ts
./api/solana/api/solanaGetPublicKey.ts
./api/solana/api/solanaSignTransaction.ts
./api/stellar/api/stellarGetAddress.ts
./api/stellar/api/stellarSignTransaction.ts
./api/stellar/stellarSignTx.ts
./api/tezos/api/tezosGetAddress.ts
./api/tezos/api/tezosGetPublicKey.ts
./api/tezos/api/tezosSignTransaction.ts
./api/tezos/tezosSignTx.ts
./api/unlockPath.ts
./api/verifyMessage.ts

Skipped for now, dependens on blockchain-link

./api/bitcoin/inputs.ts
./api/bitcoin/outputs.ts
./api/blockchainEstimateFee.ts
./api/blockchainGetAccountBalanceHistory.ts
./api/blockchainGetCurrentFiatRates.ts
./api/blockchainGetFiatRatesForTimestamps.ts
./api/blockchainGetTransactions.ts
./api/blockchainSetCustomBackend.ts
./api/blockchainSubscribe.ts
./api/blockchainSubscribeFiatRates.ts
./api/blockchainUnsubscribe.ts
./api/blockchainUnsubscribeFiatRates.ts
./api/cardano/api/cardanoComposeTransaction.ts
./api/composeTransaction.ts
./api/getAccountInfo.ts
./api/setProxy.ts
./api/signTransaction.ts

No validation currently used

./api/backupDevice.ts
./api/checkFirmwareAuthenticity.ts
./api/getDeviceState.ts
./api/getFeatures.ts
./api/getSettings.ts
./api/rebootToBootloader.ts
./api/showDeviceTutorial.ts
./api/wipeDevice.ts

resolve #5297

Copy link

socket-security bot commented Dec 11, 2023

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@sinclair/typebox-codegen 0.8.13 eval +1 704 kB sinclair
ts-mixer 6.0.3 None +0 84 kB tannerntannern
@sinclair/typebox 0.31.28 None +0 536 kB sinclair

Copy link

socket-security bot commented Dec 11, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@martykan
Copy link
Member Author

Related PR 782

@martykan martykan force-pushed the feat/schema-system-refactor branch 4 times, most recently from 48b209f to a2f2497 Compare December 19, 2023 13:08
@martykan martykan marked this pull request as ready for review December 20, 2023 08:47
Copy link
Contributor

@mroz22 mroz22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but big
image

  • left just few comments to discuss now
  • please remove commits for connect explorer, lets make a separate PR from it. no need to split our focus too much here.

also @marekrjpolak is going to read this a bit today.

package.json Show resolved Hide resolved
packages/connect-explorer/package.json Outdated Show resolved Hide resolved
packages/connect/src/device/DeviceCommands.ts Outdated Show resolved Hide resolved
packages/connect/src/constants/index.ts Outdated Show resolved Hide resolved
packages/connect/src/api/signMessage.ts Show resolved Hide resolved
@martykan martykan force-pushed the feat/schema-system-refactor branch 2 times, most recently from d75bbaf to a9a657e Compare December 20, 2023 10:09
@tsusanka tsusanka removed their request for review December 20, 2023 11:37
Copy link
Contributor

@marekrjpolak marekrjpolak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested it in runtime and also haven't checked whether the types and validations are strictly equivalent with the old ones (well, we already know they're not, but you know what I mean), let's ask the QA to test it thoroughly.

But, apart from the mostly nitpicks mentioned, I'm impressed. The typebox lib looks solid, have no other dependencies, and the Assert approach seems really easy-to-use. Good job!

Cc: @mroz22

Comment on lines +20 to +26
return true;
} catch (e) {
return false;
}
}

export function Assert<T extends TSchema>(schema: T, value: unknown): asserts value is Static<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kudos for first usage of asserts keyword in this repo, I love it already.

packages/schema-utils/src/codegen.ts Show resolved Hide resolved
@@ -0,0 +1,2611 @@
// This file is auto generated from data/messages/message.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's just my pet peeve, but I don't think it's necessary to have 5,3k lines of fixtures for this unit test. Does it make sense to prune it somehow?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this test basically takes the entire messages.ts and compares it to a snapshot. It was more for development purposes because I was interested in messages.ts in particular

packages/schema-utils/src/codegen.ts Show resolved Hide resolved
packages/connect/src/api/requestLogin.ts Outdated Show resolved Hide resolved
@mroz22 mroz22 mentioned this pull request Jan 2, 2024
5 tasks
Copy link
Contributor

@mroz22 mroz22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱
image

Comment on lines +22 to +29
export type InputWithPathParam = Static<typeof InputWithPath>;
export const InputWithPathParam = Type.Composite([
PROTO.CardanoTxInput,
Type.Object({
path: Type.Optional(DerivationPath),
}),
]);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is newly added and only used in this file. exporting it doesnt seem needed.

Copy link
Contributor

@mroz22 mroz22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am progressing but its killing me 🤯
image

just few notes..

Copy link
Contributor

@mroz22 mroz22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finally done reading. lets jump on a call and discuss few notes. we are almost there.

packages/connect/src/types/fees.ts Show resolved Hide resolved
packages/schema-utils/src/codegen.ts Show resolved Hide resolved
@martykan martykan force-pushed the feat/schema-system-refactor branch 3 times, most recently from ea26771 to e8e0c3e Compare January 5, 2024 10:07
chore(protobuf): remove `bytebuffer` dependency
fix(connect): use assert, update TS references, lint issue
fix(connect-explorer): remove old references in tsconfig
fix: update yarn, schema after rebase
fix: update yarn lock after rebase
refactor(connect): validation in ethereumSignTypedData
refactor(connect): validation in `bitcoin/refTx`
refactor(connect): simple API methods
refactor(connect): additional device API methods
fix(connect): missing validation in `requestLogin` async
refactor(connect): additional ethereum API methods
refactor(connect): validation in `signMessage/verifyMessage` API
refactor(connect): validation in methods with bundled params
refactor(connect): validation in altcoin `GetAddress` APIs

refactor(connect): validation in `GetPublicKey` APIs

refactor(connect): validation in `authenticateDevice`, `eos`, `binance`, `ethereum` APIs

refactor(connect): validation in altcoin `signTransaction` APIs
refactor(connect): validation in `cardano` APIs
refactor(connect): validate with schema in authorizeCoinjoin API
fix(connect): fix types, re-enable assert in `typedCall`
fix(connect): minor lint and type issues
fix(connect): unit test related to loosened `TxOutputType`, notes about issue trezor#10474
fix(connect): `firmwareUpdate` - version array length must be 3

fix(connect): add comments
Copy link
Contributor

@mroz22 mroz22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, lets merge this after connect 9.1.8 is ready.

@mroz22 mroz22 merged commit c5e9d50 into trezor:develop Jan 5, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

@trezor/connect: improve validateParams function
3 participants