-
Notifications
You must be signed in to change notification settings - Fork 75
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
Comments
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
We have a neat pattern we agreed on early in
pkg/sortition
where we define the local chain implementation inpkg/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
inpkg/tbtc/internal/local.go
, then the structure declared in this internal package must refer totbtc
chain types used bytbtc.Chain
. On the other hand, the unit tests living intbtc
package must create an instance oflocal.Chain
. This is why this pattern is used only inpkg/sortition
wheresorition.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 oftbtc
package. This would allow us to avoid import cycles. The same pattern is used ingo-ethereum
:core/types
orbeacon/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
andmaintainer
packages underpkg/tbtc
, moving the existingpkg/tbtc
code intopkg/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.
The text was updated successfully, but these errors were encountered: