move dcr related functionality to it's own package #257

dreacot · 2022-07-21T22:47:06Z

This PR moves all DCR related methods and data structure into it's own dcr package, assets are now located under the wallets package.

- uncomment some multiwallet method and convert then to wallet methods

dmigwi

I am still new around and I am yet to understand the best practices recommended on this project. Below are my two cents after reviewing the code.

Most newly added functions don't have descriptive comments on what the underlying code does. This becomes harder to evaluate if the specific function has all the necessary implementations without any extras.
Variable names are overly descriptive making them too long. This makes most simple implementations break the standard 80 characters a line thus the code is less legible and harder to identify errors/bugs.
Variables and functions chaining inside several structs feels excessive on most instances

dmigwi · 2022-08-12T00:11:30Z

dcr.go

+	rootDir = filepath.Join(rootDir, netType, "dcr")
+	err := os.MkdirAll(rootDir, os.ModePerm)
+	if err != nil {
+		return mwDB, "", errors.Errorf("failed to create dcr rootDir: %v", err)


Since mwDB is not assigned, wouldn't it be easier to return nil.

returning nil would give you an error in that instance which is why I initialized an empty instance at the top

dmigwi · 2022-08-12T00:12:55Z

dcr.go

+)
+
+func initializeDCRWallet(rootDir, dbDriver, netType string) (*storm.DB, string, error) {
+	var mwDB *storm.DB


you could drop this and instead make the following change at line 30.

dmigwi · 2022-08-12T00:13:41Z

dcr.go

+		return mwDB, "", errors.Errorf("failed to init dcr logRotator: %v", err.Error())
+	}
+
+	mwDB, err = storm.Open(filepath.Join(rootDir, walletsDbName))


mwDB, err := storm.Open(filepath.Join(rootDir, walletsDbName))

dmigwi · 2022-08-12T00:46:18Z

wallets/dcr/rescan.go

+			wallet.blocksRescanProgressListener.OnBlocksRescanStarted(walletID)
+		}
+
+		progress := make(chan w.RescanProgress, 1)


Isn't this as the same as making a non-buffered channel?
i.e. progress := make(chan w.RescanProgress)

as of now, i'm not writing any new code, i just moved all the old code into a package, perhaps we can consider this during a code cleanup

dmigwi · 2022-08-12T00:58:15Z

wallets/dcr/rescan.go

+		}
+
+		progress := make(chan w.RescanProgress, 1)
+		go wallet.Internal().RescanProgressFromHeight(ctx, netBackend, startHeight, progress)


There is the potential of a panic if wallet.Internal() returns a nil wallet instance.

Here is the culprit.

func (wallet *Wallet) Internal() *w.Wallet { lw, _ := wallet.loader.LoadedWallet() return lw }

It returns a new wallet instance instead of updating the old one with the latest changes.

as of now, i'm not writing any new code, i just moved all the old code into a package, perhaps we can consider this during a code cleanup

dmigwi · 2022-08-12T01:18:08Z

wallets/dcr/sync.go

+		return 0
+	}
+
+	_, height := wallet.Internal().MainChainTip(wallet.ShutdownContext())


Does making two calls to wallet.Internal() make any difference here?

dmigwi · 2022-08-12T01:19:02Z

wallets/dcr/sync.go

+	ctx := wallet.ShutdownContext()
+	_, height := wallet.Internal().MainChainTip(ctx)
+	identifier := w.NewBlockIdentifierFromHeight(height)
+	info, err := wallet.Internal().BlockInfo(ctx, identifier)


Here you've made 3 calls to wallet.Internal()

same comments as above

dmigwi · 2022-08-12T01:21:31Z

wallets/dcr/syncnotification.go

+			w.handlePeerCountUpdate(peerCount)
+		},
+		PeerDisconnected: func(peerCount int32, addr string) {
+			w.handlePeerCountUpdate(peerCount)


This is really confusing how PeerConnected and PeerDisconnected execute the same function with no changes to inputs. It be easier to just execute one since both of them might not return any difference.

reducing this unnecessary calls would reduce the number of mutex blocks requested especially on functions that require mutex blocks.

Also if only you exported all those functions in type Notification struct{}, this whole implementation wouldn't be necessary that is unless there is something I am missing out on.

dmigwi · 2022-08-12T01:27:01Z

wallets/dcr/syncnotification.go

+	w.syncData.activeSyncData.cfiltersFetchProgress.totalFetchedCFiltersCount += endCFiltersHeight - startCFiltersHeight
+
+	totalCFiltersToFetch := w.getBestBlock() - w.syncData.activeSyncData.cfiltersFetchProgress.startCFiltersHeight
+	// cfiltersLeftToFetch := totalCFiltersToFetch - w.syncData.activeSyncData.cfiltersFetchProgress.totalFetchedCFiltersCount


I presume this commented code is useless, why not delete it?

thanks for pointing out, should be uncommented

dmigwi · 2022-08-12T01:34:47Z

wallets/dcr/sync.go

+		GeneralSyncProgress:       &GeneralSyncProgress{},
+		addressDiscoveryStartTime: -1,
+		totalDiscoveryTimeSpent:   -1,
+	}


Where addition is conducted later on this -1 default values should be compensated later on to avoid misrepresenting the actual figures.

…et to hold mutiple assets, fix crash while creating a wallet

dmigwi

Since no major code changes happened, this can go in as it is!

Great work in getting this done this fast!

itswisdomagain

First pass; initial thoughts.

In addition to these, I've also not seen how to create a new dcr wallet or restore from seed and add the wallet to the multiwallet's list of dcr wallets. I was expecting to see something like mw.AddDcrWallet that creates or restores a dcr wallet and adds it to the multiwallet's list of wallets.

itswisdomagain · 2022-08-22T21:28:44Z

dcr.go

+	"github.com/planetdecred/dcrlibwallet/wallets/dcr"
+)
+
+func initializeDCRWallet(rootDir, dbDriver, netType string) (*storm.DB, string, error) {


This doesn't really initialize a dcr wallet; the databases initialized here aren't specifically used for a single dcr wallet, one is used to store multiwallet data including ALL (dcr) wallets info and also general, non-wallet specific config data. This is really actually initializing the multiwallet. It also even initializes the log rotator which is not tied to any wallet in particular but is meant for all logging done by this library. I'd say let's move the body of this function back to the NewMultiwallet function, and also retain the rootDir without appending "dcr".

I suppose we'd need a function like this in the dcr package, perhaps a InitializeWallet function so it reads as dcr.InitializeWallet. I'm not yet completely sure what the dcr.InitializeWallet function would do but it would be quite similar to what we have here. It wouldn't/shouldn't initialize a new log rotator though, the caller of the function should provide a logger to use. It also likely won't need to init a politeia database since politeia data won't be stored per wallet.

appending "dcr" enables us keep all dcr database files under a single folder and seems easier to manage/track. I agree the method name is icky, and doesn't properly describe what's going on.

having separate rootdir for different assets looks advantageous in terms of folder structure

also do you think it's remotely possible we might want to be using different database drivers for different coins?

having separate rootdir for different assets looks advantageous in terms of folder structure

I agree. What I was getting at is, the databases created here aren't even for all dcr wallets alone but for all wallets. It's the multiwallet parent database that tracks ALL wallets for all assets, which is used to populate/restore all the asset wallets when multiwallet is initialized. It also stores general config values that aren't related to any wallet.

The "dcr" subfolder should be reserved for dcr wallet databases, as in your screenshot. But there should also exist in the rootDir, a multiwallet.db file (currently called wallets.db, as in storing info for all wallets, which makes sense too).

do you think it's remotely possible we might want to be using different database drivers for different coins?

Yes. Very possible.

itswisdomagain · 2022-08-22T21:30:46Z

multiwallet.go

+	DbDriver string
+	RootDir  string
+	DB       *storm.DB

-	blocksRescanProgressListener     BlocksRescanProgressListener
-	accountMixerNotificationListener map[string]AccountMixerNotificationListener
+	ChainParams *chaincfg.Params
+	Assets      *Assets


Do these properties NEED to be exported? They were private properties.

assets would need to be if we are going to access it's fields

itswisdomagain · 2022-08-22T21:31:55Z

multiwallet.go

+				DBDriver    string
+				RootDir     string
+				DB          *storm.DB
+				ChainParams *chaincfg.Params


These aren't set or used yet. Can be removed till if and when they're needed.

db driver is not used, would remove that, the others are being used

itswisdomagain · 2022-08-22T21:33:40Z

multiwallet.go

-
-	// init database for saving/reading proposal objects
-	err = mwDB.Init(&Proposal{})
+	dcrDB, dcrRootDir, err := initializeDCRWallet(rootDir, dbDriver, netType)


Related to my first comment, this db isn't a dcrDB; it's a multiwallet db and should use the rootDir without appending "dcr". This db would track all wallets for all assets but the individual wallet dbs would be in the asset subfolders.

itswisdomagain · 2022-08-22T21:36:25Z

multiwallet.go

+		SetLogLevels(logLevel)
+
+		// initialize Politeia.
+		wallet.NewPoliteia(politeiaHost)


wallet.NewPoliteia returns some values that currently ignored.
Also, politeia shouldn't be a wallet construct - it's not tied to any one wallet. It should be much like the internal/vsp package that is pretty much standalone but can work with any provided dcr wallet.

that true, also i agree we shouldn't have politeia as a wallet struct, current problem is that politeia is now a wallet property, viewable when a dcr wallet is opened, and each dcr wallet can have it's own specific settings in regards to politeia.

but with the new internal politeia package that was merged, we'd have to take away this code anyways

dreacot · 2022-08-26T14:13:45Z

First pass; initial thoughts.

In addition to these, I've also not seen how to create a new dcr wallet or restore from seed and add the wallet to the multiwallet's list of dcr wallets. I was expecting to see something like mw.AddDcrWallet that creates or restores a dcr wallet and adds it to the multiwallet's list of wallets.

Previous implementation didn't make room for adding to the multiwallet wallets list

dreacot force-pushed the dcr_package branch 2 times, most recently from cc76042 to a649063 Compare July 21, 2022 22:58

move dcr related capability to the dcr package

2b0f810

dreacot force-pushed the dcr_package branch from a649063 to 2b0f810 Compare July 21, 2022 22:59

dreacot added 7 commits July 28, 2022 12:31

- move politeia to dcr package

4ef5384

- uncomment some multiwallet method and convert then to wallet methods

remove wallet specific methods from multiwallet

e1dac38

fix errors in dcr/sync.go

e583a49

fix bug where block info was being returned instead of int

07d948e

fix compilation errors in dcr/ticket.go

48f7691

remove multiwallet method from dcr/txauthor

ab7707d

convert dcr/vsp.go multiwallet methods to wallet methods

25f756a

dreacot marked this pull request as ready for review July 28, 2022 22:55

removed unused comments

8e33f3e

dreacot changed the title ~~move dcr related functionlaity to it's own package~~ move dcr related functionality to it's own package Jul 28, 2022

fix crash that occured while trying to create a wallet

75cf4ff

dmigwi reviewed Aug 12, 2022

View reviewed changes

dreacot added a commit to song50119/dcrlibwallet that referenced this pull request Aug 15, 2022

- rebase with planetdecred#257(dcr package PR), restructure multiwall…

d879adf

…et to hold mutiple assets, fix crash while creating a wallet

dreacot added a commit to song50119/dcrlibwallet that referenced this pull request Aug 18, 2022

- rebase with planetdecred#257(dcr package PR), restructure multiwall…

dc3e86c

…et to hold mutiple assets, fix crash while creating a wallet

manage coins under an Asset struct

490aa64

dmigwi approved these changes Aug 22, 2022

View reviewed changes

itswisdomagain suggested changes Aug 22, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move dcr related functionality to it's own package #257

move dcr related functionality to it's own package #257

dreacot commented Jul 21, 2022

dmigwi left a comment

dmigwi Aug 12, 2022

dreacot Aug 22, 2022

dmigwi Aug 12, 2022

dmigwi Aug 12, 2022

dmigwi Aug 12, 2022 •

edited

Loading

dreacot Aug 22, 2022

dmigwi Aug 12, 2022

dreacot Aug 22, 2022

dmigwi Aug 12, 2022 •

edited

Loading

dmigwi Aug 12, 2022

dreacot Aug 22, 2022

dmigwi Aug 12, 2022

dmigwi Aug 12, 2022

dmigwi Aug 12, 2022

dmigwi Aug 12, 2022

dreacot Aug 22, 2022

dmigwi Aug 12, 2022

dmigwi left a comment

itswisdomagain left a comment

itswisdomagain Aug 22, 2022

itswisdomagain Aug 22, 2022

dreacot Aug 23, 2022

itswisdomagain Aug 24, 2022

itswisdomagain Aug 22, 2022

dreacot Aug 26, 2022

itswisdomagain Aug 22, 2022

dreacot Aug 26, 2022

dreacot Aug 26, 2022

itswisdomagain Aug 22, 2022

itswisdomagain Aug 22, 2022

dreacot Aug 26, 2022

dreacot commented Aug 26, 2022

move dcr related functionality to it's own package #257

Are you sure you want to change the base?

move dcr related functionality to it's own package #257

Conversation

dreacot commented Jul 21, 2022

dmigwi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmigwi Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmigwi Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmigwi left a comment

Choose a reason for hiding this comment

itswisdomagain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreacot commented Aug 26, 2022

dmigwi Aug 12, 2022 •

edited

Loading

dmigwi Aug 12, 2022 •

edited

Loading