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

Consistent approach for local chain implementations and chain types #3652

Open
pdyraga opened this issue Jun 27, 2023 · 0 comments
Open

Consistent approach for local chain implementations and chain types #3652

pdyraga opened this issue Jun 27, 2023 · 0 comments

Comments

@pdyraga
Copy link
Member

pdyraga commented Jun 27, 2023

We have a neat pattern we agreed on early in pkg/sortition where we define the local chain implementation in pkg/internal/local. This way, _test.go is only for actual tests and we can unit-test the local chain implementation as well.

Unfortunately, this pattern can cause import cycles. Suppose we implement tbtc.Chain in pkg/tbtc/internal/local.go, then the structure declared in this internal package must refer to tbtc chain types used by tbtc.Chain. On the other hand, the unit tests living in tbtc package must create an instance of local.Chain. This is why this pattern is used only in pkg/sortition where sorition.Chain does not declare any chain types.

One way to overcome this problem is to define chain types in a separate package as proposed here. pkg/tbtc/types would contain all types defined by the chain interface of tbtc package. This would allow us to avoid import cycles. The same pattern is used in go-ethereum: core/types or beacon/types. There are some concerns about bending the rule and referring to a sub-package from an upper-level package though.

Another way to overcome this problem is to de-flatten the structure of our packages and put the coordinator and maintainer packages under pkg/tbtc, moving the existing pkg/tbtc code into pkg/tbtc/wallet. There seems to be a broad agreement we are not in favor of this option given we like the flat structure of packages.

Yet one option is to declare a separate tbtccore package but there are concerns this would be a common-like package evolving over time into a monster structure that is hard to control.

The discussion: #3650 (comment)

As a part of the work on this issue, we should also cleanup how mutexes work in local chain implementations. We should have a single per-struct mutex instead of defining them per field. This approach clutters the code and does not provide any significant boost for the unit test execution time.

pdyraga added a commit that referenced this issue Jun 29, 2023
Here we remove the `pkg/coordinator` by moving their contents to
`pkg/maintainer/wallet`. This package seems to be no longer needed so
this operation makes the package structure and dependency graph simpler.
This refactoring was done in several steps:

### Step 1: Make the nomenclature in `pkg/maintainer/wallet` more
precise

The tBTC v2 system defines two types of sweeps: deposit sweeps and moved
funds sweeps. In the first step, we narrowed the existing nomenclature
used within the wallet maintainer to remove ambiguity and make it clear
that the existing code refers to deposit sweeps.

### Step 2: Move the deposit sweep code from `pkg/coordinator` to
`pkg/maintainer/wallet` package

In the second step, we moved the deposit sweep code living so far in the
`pkg/coordinator`. By the way, some
simplifications were made in order to make the public API cleaner and
more readable. After this change, the `pkg/maintainer/wallet` package
exposes the following public API:
- `FindDeposits` that allows to find deposits according to the given
criteria
- `FindDepositsToSweep` that finds a batch of deposits that are most
suitable to become part of a deposit sweep
- `ProposeDepositsSweep` which submits a deposit sweep proposal to the
`WalletCoordinator` contract
- `EstimateDepositsSweepFee` that estimates the deposit sweep
transaction fee
- `DepositReference` which is a set of data allowing to identify and
refer to a deposit
- `Deposit` that holds some detailed data about a deposit
- `ParseDepositsReferences` that allows creating an array of
`DepositReference` based on the provided input string
- `ValidateDepositReferenceString` that allows to validate whether the
given string can be used to create a valid `DepositReference` instance

This step also involved moving unit tests stressing the deposit sweep
code described above.

### Step 3: Move the redemption code from `pkg/coordinator` to
`pkg/maintainer/wallet` package

In this step, we moved the redemption code from `pkg/coordinator` to the
`pkg/maintainer/wallet` package. This was similar to the previous step
but much simpler. After this step, the public API of the
`pkg/maintainer/wallet` package was extended with the following items:
- `FindPendingRedemptions` which allows to find pending redemption
requests
- `ProposeRedemption` which submits a redemption proposal to the
`WalletCoordinator` contract
- `EstimateRedemptionFee` that estimates the redemption transaction fee

This step also involved moving unit tests stressing the redemption code
described above.

### Step 4: Move the `coordinator.Chain` interface

In the fourth step, we moved the `coordinator.Chain` interface by
merging it with the `maintainer/wallet.Chain` interface.
This change clarifies the chain expectations defined by the
`pkg/maintainer/wallet` package. We also moved the remaining chain types
living so far in the `pkg/coordinator` (`coordinator.
NewWalletRegisteredEvent` and `coordinator.
NewWalletRegisteredEventFilter`) to the `pkg/tbtc` package. This package
should be the right place for them according to the recent discussion
(see
#3650 (comment)),
at least until #3652 is
done.

### Step 5: Remove `pkg/coordinator` package

In the last step, we removed the `pkg/coordinator` package and made sure
all references from the `cmd` package were correctly replaced.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant