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

Repo package layout refactoring: determine which packages should be exported as vgo modules #1147

Closed
jrick opened this issue May 25, 2018 · 5 comments

Comments

@jrick
Copy link
Member

jrick commented May 25, 2018

dcrwallet is not just a program -- like dcrd, it also includes packages that can be directly imported by other tools for integrating various wallet functionality. For example, lnd imports the wallet package in order to implement key storage and signing. However, not all packages are designed for external consumption, and there should be a clear distinction between which packages must follow strict semantic versioning and those which don't. This issue provides a discussion to determine which packages should be created into modules, which should not, and whether the package layout must adjust to accommodate.

The following are all of the packages imported or used by the main package, and the repo's root main package itself (all of which make up the dcrwallet executable):

                "github.com/decred/dcrwallet",
                "github.com/decred/dcrwallet/chain",
                "github.com/decred/dcrwallet/errors",
                "github.com/decred/dcrwallet/internal/cfgutil",
                "github.com/decred/dcrwallet/internal/helpers",
                "github.com/decred/dcrwallet/internal/prompt",
                "github.com/decred/dcrwallet/internal/rpchelp",
                "github.com/decred/dcrwallet/internal/zero",
                "github.com/decred/dcrwallet/loader",
                "github.com/decred/dcrwallet/netparams",
                "github.com/decred/dcrwallet/pgpwordlist",
                "github.com/decred/dcrwallet/rpc/legacyrpc",
                "github.com/decred/dcrwallet/rpc/rpcserver",
                "github.com/decred/dcrwallet/rpc/walletrpc",
                "github.com/decred/dcrwallet/snacl",
                "github.com/decred/dcrwallet/ticketbuyer",
                "github.com/decred/dcrwallet/version",
                "github.com/decred/dcrwallet/wallet",
                "github.com/decred/dcrwallet/wallet/internal/txsizes",
                "github.com/decred/dcrwallet/wallet/txauthor",
                "github.com/decred/dcrwallet/wallet/txrules",
                "github.com/decred/dcrwallet/wallet/udb",
                "github.com/decred/dcrwallet/walletdb",
                "github.com/decred/dcrwallet/walletdb/bdb",
                "github.com/decred/dcrwallet/walletseed",

Some of my own observations/thoughts/rambling follow. Feel free to follow up with comments whether you agree, disagree, or have suggestions. Once thoughts are fleshed out, a new repo/module layout can be proposed.

  1. The repository currently makes use of internal packages. These are packages which can not be imported by any other packages without the same package prefix (up to the /internal). If they are included in a module, they do not make up the module's external API and compatibility may be broken without creating a new major version of the module. It can be advantageous to increase the number of internal packages in order to reduce a module's exported API.

  2. Some packages must be available for consumption by other projects. I believe this must include (at minimum) errors, pgpwordlist and walletseed, version, wallet, wallet/txauthor, wallet/txrules, wallet/internal/txsizes, and chain. Rationale for each follows:

  • errors implements error handling throughout the project, and errors returned by modules must be compared using this package.

  • pgpwordlist and walletseed are each small packages and together implement encoding and decoding wallet seeds in hex or PGP word list format. They do not depend on any other wallet packages (besides errors) and would be trivial to make modules with semver guarantees.

  • version implements the release version of the dcrwallet main package. This is useful for build systems which must include the release version of dcrwallet, as storing this information in the main package would prevent it from being imported. While it may be useful for external consumption, as it describes the dcrwallet version, perhaps it should remain under the root module.

  • wallet is easily the most interesting package for external projects. It provides nearly all of the essential wallet functionality (managing transactions, authoring transactions, addresses, keys, signing, etc.). I say nearly all functionality as this package does not provide, by itself, any way to interact with the Decred network. That functionality is currently provided by a wallet.NetworkBackend interface which is implemented by chain, and soon to be implemented by the spv package as well. wallet also exposes one of the largest package APIs in the project, and I expect that providing strict semver backwards compatibility may result in many major version bumps as the API is cleared of years of unfortunate decisions.

  • wallet/txauthor and wallet/txrules, while under the wallet package path, are each standalone packages which may prove useful to other projects. txauthor provides a method of creating unsigned transactions over any UTXO set, and txrules describes basic consensus and policy rules, such as the transaction relay fee and dust threshold.

  • wallet/internal/txsizes is not currently available to be imported by external projects, but this would be a good opportunity to make it so. The package implements estimation calculations of transaction sizes given various numbers of inputs and outputs. It is currently an internal package since it makes assumptions regarding the precise types of inputs being used by the wallet to construct the transaction, and therefore is not suitable for all transaction size estimates. However, it is still useful, and as long as the API is modified to reflect its shortcomings, it can be exported as or as part of a module.

  • chain (and spv in the future) provide an implementation of wallet.NetworkBackend and implement the ability to keep a wallet in sync with the Decred network. This is desirable for external projects if they need to care about more than just offline signing, such as details about the current blockchain, wallet UTXO set, transactions, double spend detection, and more.

  1. Some external projects may want to depend on packages in submodules not in the above list. I currently see this group made up of packages such as ticketbuyer and loader. I'm open for future support for making modules out of these packages, but do not deem it a priority until the other packages have been modularized. Both packages depend heavily on the wallet and network abstractions. I would especially like to make loader a public module off the bat, as it provides a simpler way to manage the lifetime of a wallet, but this package exports ticketbuyer and should wait until ticketbuyer becomes a module.

  2. There are packages which likely should be internal packages but are not, either due to accident or necessity. For example, the github.com/decred/dcrwallet/wallet/udb package provides the implementation for storing and retrieving all database data for the github.com/decred/dcrwallet/wallet package. This is an internal implementation detail, and udb should not be available to import by other projects. At the moment, there exists a layering violation where udb types are exported by the wallet package and consumed by other packages such as the RPC servers, which is why udb is currently not an internal package. snacl is another good candidate to make internal, and would be easier to do than udb as it is not exported by any other proposed module. In addition to these, there are other packages, such as the service implementations of the RPC servers, which should not be imported by external projects as they are only used by the root module, but it is unclear to me at this point whether these should be made internal packages of the root module or left alone.

@jrick
Copy link
Member Author

jrick commented May 25, 2018

walletdb is an interesting package to deal with as well. It (specifically, the walletdb.DB type) is exported by the wallet package so that wallets can be created in, written to, and opened from a database. Because the callers must handle the DB, this package can not simply be made internal. However, all of the methods of the walletdb.DB interface do not need to be available to consumers of the module; only the wallet (and udb) packages need these methods.

There are two possible improvements I see here. Either the walletdb.DB type can become an opaque type that can refer to a database, and provides the wallet and udb packages with the methods needed, but without exposing those methods to consumers of the module. A second way (which may not work, and needs further investigation) could be to refer to the database by the filename rather than the database object itself.

@jrick
Copy link
Member Author

jrick commented May 26, 2018

#1151 moves walletdb to become a wallet internal package and introduces an opaque type to allow the database to continue to be referenced by loader and main.

@kLkA
Copy link
Contributor

kLkA commented Jun 9, 2018

#1172 moves snacl package inside wallet internal packages dir

@kLkA
Copy link
Contributor

kLkA commented Jun 9, 2018

#1173 moves txsizes package from wallet/internal to wallet package

Additionaly, I'd open discussion about txauthors, txrules and txsizes imports.

"github.com/decred/dcrd/dcrutil"
"github.com/decred/dcrd/wire"
"github.com/decred/dcrd/chaincfg"
"github.com/decred/dcrd/chaincfg/chainec"
"github.com/decred/dcrd/blockchain"

"github.com/decred/dcrwallet/errors"
h "github.com/decred/dcrwallet/internal/helpers"

I'd like to propose to consolidate 3 mentioned packages as a single txutils vgo module
Only internal/helpers stops me from suggest to move it out of wallet package
which could become txutils/helpers.go func.
Sum helper funcs may also be helpful for someone using txutils to combine use in tx construction

@jrick
Copy link
Member Author

jrick commented Aug 30, 2018

The repo has been split into various modules with #1238.

@jrick jrick closed this as completed Aug 30, 2018
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

2 participants