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

General revamp of the typescript library #705

Merged
merged 44 commits into from
Oct 16, 2023
Merged

General revamp of the typescript library #705

merged 44 commits into from
Oct 16, 2023

Conversation

lukasz-zimnoch
Copy link
Member

@lukasz-zimnoch lukasz-zimnoch commented Sep 29, 2023

Closes: #698

Here we present a comprehensive refactoring of the tBTC v2 typescript library (a.k.a. typescript SDK). So far, the library was basically a bag of various utility functions, and the responsibility of using them correctly was on the library clients' side. This approach was enough for the first phases of protocol development where the library was used mainly for internal purposes. However, as the protocol becomes more mature, it makes sense to encourage external integrators. This, in turn, requires us to improve the developer experience provided by the SDK and reduce the overhead for users. This pull request aims to achieve that goal.

The initial plan was to open several PRs one after another but, given the fact that #695 was happening simultaneously and had precedence, it was easier to track progress and resolve conflicts within a single pull request. Because of the size, I strongly recommend reviewing commit by commit. The history of changes clearly shows all the steps that were taken. Worth noting that there are very few changes in the logic itself. Most of them are file/directory structure changes, renames, and public API improvements. Here is a detailed description of specific steps:

Rework of the file and directory structure

So far, the file structure of the library was flat and grouped related functions into files like bitcoin.ts or deposit.ts. This quickly led to poor readability and encapsulation. The new approach uses a hierarchical approach dividing code into two distinct categories:

  • Shared libraries living under lib directory
  • Feature services living under services directory

Shared libraries

This category groups all code components that are used across different features. Specific modules are:

  • lib/bitcoin: Contains all interfaces and utilities necessary to interact with the Bitcoin chain
  • lib/contracts: Provides chain-agnostic interfaces allowing to interact with tBTC v2 smart contracts
  • lib/electrum: Contains an Electrum-based implementation of the Bitcoin client
  • lib/ethereum: Provides implementations of tBTC v2 smart contract interfaces for Ethereum chain
  • lib/utils: Holds some general utility functions

Each aforementioned module contains an index.ts file that re-exports components from specific sub-modules in order to flatten import paths used by library clients.

Feature services

This category holds services providing access to specific features of the tBTC v2 system:

  • services/deposits: Provides access to the tBTC v2 deposit flow for BTC depositors through the DepositsService component
  • services/redemptions: Provides access to the tBTC v2 redemption flow for TBTC redeemers through the RedemptionsService component
  • services/maintenance: Provides access to authorized maintenance actions for maintainers (e.g. optimistic minting guardians) through the MaintenanceService component

Just as for shared libraries, each module contains an index.ts that re-exports all exported components from specific sub-modules.

Provide a simple entry point with embedded chain configurations

So far, the library has not provided any unified entry point that could be used by external clients. The root index.ts just exported the most important components without any further guidance. Here we aim to change that by introducing the TBTC entry point class. The main purpose of it is to provide some static functions allowing to quickly initialize the SDK and expose specific feature services in a simple way. As for now, the TBTC component can be initialized using the following functions:

  • initializeMainnet which initializes the SDK to point tBTC v2 contracts on Ethereum and use mainnet as Bitcoin network
  • initializeGoerli which initializes the SDK to point tBTC v2 contracts on Goerli and use testnet as Bitcoin network
  • initializeCustom which allows using custom tBTC v2 contracts (e.g. local) and use an arbitrary Bitcoin network (e.g. regtest or simnet)

Once initialized, the TBTC component provides easy access to specific feature services. To address unforeseen needs, it also gives low-level direct access to the underlying tBTC v2 smart contracts and the Bitcoin client:

// Initialize SDK
const signer = (...) 
const sdk = await TBTC.initializeMainnet(signer) 

// Access feature services
sdk.deposits.(...)
sdk.redemptions.(...)

// Access tBTC v2 smart contracts and Bitcoin client directly
sdk.tbtcContracts.bridge.(...)
sdk.bitcoinClient.(...)

Other changes

Apart from the ones mentioned above, there were other important changes that had to be made. First and foremost, this PR integrates bcoin removal done as part of #695. The integration was achieved by applying changes from specific PRs in separate commits here. Secondly, the unit tests file structure was adjusted to reflect the new approach with lib and services top-level directories.

Usage examples

Changes made as part of this work greatly facilitated access to the two core use cases of the tBTC v2 system: depositing and redeeming. Here are some examples of how it looks like now:

// Initialize SDK
const signer = (...) 
const sdk = await TBTC.initializeMainnet(signer) 

// Deposit flow
const bitcoinRecoveryAddress = "" // take this from user input
const deposit = await sdk.deposits.initiateDeposit(bitcoinRecoveryAddress)
const depositAddress = await deposit.getBitcoinAddress() // present that to the user
await deposit.initiateMinting() // this will initiate minting if BTC were sent to the deposit address

// Redemption flow
const bitcoinRedeemerAddress = "" // take this from user input
const amount = "" // take this from user input
await sdk.redemptions.requestRedemption(bitcoinRedeemerAddress, amount)

typescript/src/lib/bitcoin/csuint.ts Show resolved Hide resolved
typescript/test/bitcoin.test.ts Outdated Show resolved Hide resolved
typescript/src/lib/bitcoin/csuint.ts Outdated Show resolved Hide resolved
typescript/src/lib/bitcoin/ecdsa-key.ts Outdated Show resolved Hide resolved
typescript/src/lib/bitcoin/ecdsa-key.ts Outdated Show resolved Hide resolved
The `bitcoin.ts` file is a bag for all Bitcoin related code which makes
it hard to read and maintain. Here we split that file into smaller
ones oriented around specific Bitcoin subdomains and move them to the
new `lib/bitcoin` shared library module.
We just moved `bitcoin.ts` to `lib/bitcoin`. Here we do the same
transition for `chain.ts`.
Those types are purely chain-related and should be placed next to
the respective contracts interfaces.
Those types are purely chain-related and should be placed next to
the respective contracts interfaces.
Those types are purely chain-related and should be placed next to
the respective contracts interfaces.
Those types are purely chain-related and should be placed next to
the respective contracts interfaces.
This content of the `proof.ts` file actually belongs to `lib/bitcoin/proof.ts`.
Part of this code related to blocks, should live in `lib/bitcoin/block.ts`.
Here we make a final rework of the `lib/bitcoin` module. The main
work is focused on introducing a consistent naming system that doesn't
cause name clashes outside and group related functions into separate
components.
Here we make a final rework of the `lib/contracts` module. The main
work is focused on improving component names.
Here we make a final rework of the `lib/electrum` module. The main
work is focused on improving component names.
Here we make a final rework of the `lib/ethereum` module. The main
work is focused on improving component names and structure.
Replace current objects with `DepositReceipt` carrying state relevant
for Bitcoin and `DepositRequest` carrying state relevant for the Bridge
contract.
Here we remove the existing `deposit.ts` file, move it to the new
`./services/deposits` feature module and do a general refactoring
to make it easy to use.
This code is part of the user-facing deposit flow so should live in the
`./services/deposits` module.
…module

Move the user-facing redemption code to the new `./services/redemptions` module.
Some refactoring meant to facilitate usage was done as well.
The code left is purely related to system maintainers or is helper
tooling empowering internal operations like system testing. We
are moving it to a dedicated module.

By the way, we are arranging the main `index.ts` and adjust all imports
to use the base path and not dive into specific directories.
This test suite depends on external Electrum servers and is prone to their
health fluctuations. We observed such fluctuations now, and we are temporarily
skipping their default run to avoid loosing time.
Here we add the `TBTC` entrypoint component allowing to easily
set up the SDK to work with supported Ethereum-based and Bitcoin networks.
Here we get rid of the `depositor` parameter that must be passed by the caller
in favor of a default depositor set upon deposit service creation. This
should facilitate the public API of the deposit service and make it more
friendly for 3rd party integrators.
@lukasz-zimnoch lukasz-zimnoch marked this pull request as ready for review October 11, 2023 10:33
@nkuba nkuba requested a review from r-czajkowski October 11, 2023 10:37
typescript/package.json Outdated Show resolved Hide resolved
typescript/src/lib/bitcoin/address.ts Outdated Show resolved Hide resolved

if (witness) {
// P2WPKH (SegWit)
return payments.p2wpkh({ pubkey: publicKey.toBuffer(), network }).address!
Copy link
Member

Choose a reason for hiding this comment

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

What is returned if this line fails? Don't we want to ensure the address is defined and non-empty and throw an error otherwise?

Copy link
Contributor

@tomaszslabon tomaszslabon Oct 12, 2023

Choose a reason for hiding this comment

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

If the provided publicKey is correct, the address will always be defined and correct.
If the provided publicKey is incorrect (e.g. is empty, the length is incorrect, the prefix is incorrect) this function will throw an error:

  1) Bitcoin
       BitcoinAddressConverter
         publicKeyToAddress
           with testnet addresses
             should return correct P2PKH address for testnet:
     Error: Expected property "pubkey" of type ?isPoint, got Buffer

I'm not sure we need to add any further check on the publicKey, unless we want to have cleaner error messages (i.e. stating exactly what is wrong).
We could add unit tests for the malformed publicKey, though.
But perhaps not in this PR , as it's all about the design.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's add unit tests as part of #714 @tomaszslabon

Copy link
Contributor

Choose a reason for hiding this comment

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

@lukasz-zimnoch OK, let's not resolve this comment to keep it as a reminder.

typescript/src/lib/bitcoin/address.ts Show resolved Hide resolved
const hash = Buffer.from(publicKeyHash, "hex")
const network = toBitcoinJsLibNetwork(bitcoinNetwork)
return witness
? payments.p2wpkh({ hash, network }).address!
Copy link
Member

Choose a reason for hiding this comment

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

Same question about what happens when the address geneartion fails?

Copy link
Contributor

@tomaszslabon tomaszslabon Oct 12, 2023

Choose a reason for hiding this comment

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

If the provided publicKeyHash is incorrect (it's not a hexstring of length 20) the function will throw an error:

  1) Bitcoin
       BitcoinAddressConverter
         publicKeyHashToAddress
           when network is mainnet
             when witness option is false
               when proper public key hash is provided
                 should encode public key hash into bitcoin address properly:
     Error: Expected property "hash" of type Buffer(Length: 20), got Buffer(Length: 19)

So it either succeeds (i.e. the address is defined) or it throws.
We have some unit tests covering incorrect length.
We could replace the string argument with Hex, though.
This will ensure we won't end up with a non-hexstring character, or a string with odd number of characters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's add hex conversion as part of #713 and unit tests as part of #714

Copy link
Contributor

Choose a reason for hiding this comment

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

@lukasz-zimnoch OK, let's not resolve this comment to keep it as a reminder.

typescript/src/lib/bitcoin/csuint.ts Show resolved Hide resolved
typescript/src/lib/bitcoin/csuint.ts Show resolved Hide resolved
typescript/src/lib/bitcoin/ecdsa-key.ts Show resolved Hide resolved
typescript/src/lib/bitcoin/ecdsa-key.ts Show resolved Hide resolved
typescript/src/lib/bitcoin/ecdsa-key.ts Show resolved Hide resolved
Copy link
Member

@nkuba nkuba left a comment

Choose a reason for hiding this comment

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

Impressive work! 👏
I'm leaving the first chunk of comments.

typescript/src/lib/bitcoin/hash.ts Show resolved Hide resolved
typescript/src/lib/bitcoin/hash.ts Show resolved Hide resolved
typescript/src/lib/bitcoin/hash.ts Show resolved Hide resolved
typescript/src/lib/bitcoin/hash.ts Outdated Show resolved Hide resolved
typescript/src/lib/bitcoin/header.ts Show resolved Hide resolved
typescript/src/lib/bitcoin/header.ts Outdated Show resolved Hide resolved
typescript/src/lib/bitcoin/header.ts Show resolved Hide resolved
typescript/src/lib/bitcoin/script.ts Show resolved Hide resolved
typescript/src/lib/bitcoin/script.ts Show resolved Hide resolved
typescript/src/lib/bitcoin/tx.ts Show resolved Hide resolved
@tomaszslabon
Copy link
Contributor

tomaszslabon commented Oct 12, 2023

@lukasz-zimnoch
We could update the node version requirement in README.
Similarly here.
We require the node version to be 16 or higher.

@lukasz-zimnoch
Copy link
Member Author

here

Good catch! See 4bdc2a1

@tomaszslabon tomaszslabon merged commit 05679d1 into main Oct 16, 2023
38 checks passed
@tomaszslabon tomaszslabon deleted the ts-api-revamp branch October 16, 2023 13:29
@Shadowfiend
Copy link
Contributor

Just like the tiniest niptick here… Do we feel like services is the right term for that folder? Given services is often used to refer to longer-running server-side components (even though it has a broader meaning!), I wonder if simply component might be a term that will result in less confusion long term? I know it's a pretty internal detail, so obviously urgency is rather low, and my conviction is similarly low here—but wanted to mention it because I had a little dissonance when I saw it.

@Shadowfiend
Copy link
Contributor

Oh also, holy hell this is beautiful.

@lukasz-zimnoch
Copy link
Member Author

Just like the tiniest niptick here… Do we feel like services is the right term for that folder? Given services is often used to refer to longer-running server-side components (even though it has a broader meaning!), I wonder if simply component might be a term that will result in less confusion long term? I know it's a pretty internal detail, so obviously urgency is rather low, and my conviction is similarly low here—but wanted to mention it because I had a little dissonance when I saw it.

I was wrapping my head around this for a while as well :D My first take was modules but this is causing a lot of confusion with JavaScript modules. Not sure about components - IMO, it's pretty generic and does not fully reflect that those are components that directly face the SDK client. Totally agree that services is not ideal though. My reasoning was: this is the code that provides specific services for SDK clients, so why not call it just services? If we come up with something better here, I'm totally open to change it. The library is still evolving.

@Shadowfiend
Copy link
Contributor

Yeah I don’t love components either, but in this case it’s generic nature kind of helps with the possibility for confusion.

I feel like there’s a better word kicking around somewhere though, I’ll see if it pops into my head at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔌 typescript TypeScript library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revamp the typescript library API
4 participants