btcsuite / paymetheus Goto Github PK

The Wallet model class uses a Dictionary keyed by Account (numbers) to store account properties. When used as an IEnumerable, the order is not defined. This can cause the ObservableCollection of recent accounts in the shell view model (which does have an ordering) to order the accounts in an implementation dependent manner.

I think the best way to solve this issue is to maintain a sorted List of accounts and their properties in the Wallet class. Other than the imported account (which is planned to be removed from btcwallet in a later update), all accounts numbers are strictly increasing.

The account rename dialog appears to make an asynchronous RPC call to rename the account and close itself immediately without waiting for a response.

This causes an issue because it leads to the account name in accounts list not being updating for a couple of seconds after the dialog closes when the RPC is slow to respond with btcwallet is under heavy load (such as during initial sync). The result is it appears like the rename was simply ignored which leads to the user attempting to rename it again, except this time the dialog won't do anything when you click rename (no errors or anything), likely because it's trying to rename an account that no longer exists.

I believe the correct way to handle this is simply to have the rename dialog wait for the RPC to complete with a standard "please wait...." message.

Paymetheus should execute the wallet process with the --datadir option to set an alternate application data directory for wallet to write its data to (most importantly, the wallet DB and RPC cert). This will make Paymetheus appear more as a standalone product as it won't have any unusual interaction from an already existing wallet datadir (such as failing to start due to an already existing rpc.key, which breaks the --onetimetlskey option used by Paymetheus).

When opening the account's overview page, the number of external, internal, and imported keys managed by the account are all zero. These need to be filled with real information from the Accounts RPC.

The msi installs dcrwallet.exe in the same directory as paymetheus.exe. We should try to run dcrwallet from the same path first.

We may want to disallow running a dcrwallet from anywhere else however, that makes testing hard. Think about this some more.

This should probably go somewhere in the Create Transaction view.

There is a placeholder checkbox for this feature, but it is not implemented yet, and the checkbox does nothing.

IDE Version: Visual Studio 2015 update 3
Error message: An exception of type 'System.AggregateException' occurred in mscorlib.dll but was not handled in user code
Code: App.xaml.cs: 101

Unless it is hidden somewhere, I don't see any way to import an existing seed. A new seed is automatically created and prompted.

The intent is to only save transactions from the last 100 (coinbase maturity) blocks since these are needed to efficiently calculate a balance given any arbitrary number of confirmations, without going back over RPC.

Some of the code works with just these recent transactions available, but other parts (account history) require every transaction that is relevant to the wallet to be in Paymetheus's memory. RPC will be required to download these transactions as needed and discard when they are finished being used.

Aside from the application name, there is no branding yet. An icon is needed.

Ideally all of the non-GUI code (especially in Paymetheus.Bitcoin) has test coverage. btcsuite's Go projects contain a lot of great test vectors that can be ported over.

When a transaction is viewed from the recent history summary, or when an account is clicked on, it may be unobvious how to return to the previous view. Right now any other view can be selected by clicking that view in the sidebar, but users may find this navigation confusing.

Right now this is hardcoded as 1 block confirmation everywhere in the code. Instead, this should be user-selectable.

Some kind of settings view could be added that enables changing this option.

Changing this value should result in updating all of the spendable balances with the new required number of confs.

The largest value the required confirmations can be set to is 100 (the coinbase maturity value) since transactions mined after this many blocks are not guaranteed to be saved in memory by Paymetheus.

When Paymetheus creates a new wallet, the wallet seed is displayed in hexadecimal encoding to the user who then must type it back into a dialog box on the next page of the wizard. That confirmation check should prevent the user from continuing without the seed properly backed up, but it may take 2 or 3 tries before they get it right since writing down a bunch of hex digits on paper can be annoying and easily screwed up. Instead, some kind of mnemonic code could be used here to make the process a little easier.

Some possible options to create this code:

• BIP0039
• PGP Word List (and a Go implementation for reference)

This is something I spent significant time on but couldn't get it just the way I liked. Currently, none of the scrollable itemized lists contain any kind of column headers, and this can make the application confusing to use. As an example, when viewing an account's transaction history, Bitcoin amounts for the credit, debit, and total difference are displayed, but none of these columns are labeled.

GridView may be an option here but I would want to have it styled to look more minimal and match the current style. (Right now an unstyled GridView looks like it came straight out of Windows Aero).

This is both a btcwallet and a Paymetheus bug. btcwallet should not be allowing this (waddrmgr should error), but Paymetheus should also disable the Rename button in the dialog if the text box is empty.

btcwallet just added the VersionService to query for the RPC server's SemVer version. Paymetheus needs to query this version after the client is created to ensure that the processes are compatible with each other.

I understand that currently there probably isn't a gRPC to query if a public passphrase is needed, but ideally there should be one added. It is confusing to request a passphrase when there isn't one.

In the mean time, I think some instructions at the top of the dialog to clarify the situation would go a long way to clearing up the confusion. Perhaps something like the following:

A public passphrase is only required if you chose to encrypt public data when creating the wallet. Simply click Open Wallet if you did not encrypt public data.

This also affects the total balance across all accounts at the bottom.

The estimated fees in the account's transaction creation tab are always zero because this is unimplemented.

Additionally, every created transaction uses a hardcoded fee of 1e4. The estimated fee should be used here instead, and it should be calculated as a function of the estimated transaction size rather than simply being a constant.

There is no real version contained in the application currently (aside from a placeholder in the project's assembly info). This needs to be addressed, as well as adding a method of viewing the version of a running Paymetheus instance.

It may also be worth investigating how to use msbuild to add the git commit revision into the version name somehow.

It isn't clear from the initial connection screen that btcd has to be running and that I'm entering the credentials for it.

This could be cleared up pretty easily I think with some instructions at the top of the dialog.

This should be configurable using command line arguments. Obviously the typical user won't be running this from the command line, but multiple shortcuts could be created that call the process with different args depending on the network to use.

So this is just a quality of life issue with Paymetheus. Regarding the transaction history page. Whenever you want to check your newest transactions you need to go to the transaction history page and then you have to scroll all the way to the bottom.

Some solutions, if you think this is an issue, is to invert it so that newest transactions go at the top. You could also start the scroll bar somewhere else from the top. Or there could be a toggle on the date column to choose whether to sort by newest or oldest.

Right now, the System.String type is used for passphrases, the wallet seed, etc. This type is immutable and it is impossible (without unsafe) to clear it after it is finished being used. Instead, investigate other methods of storing these secrets (most likely a byte[]), avoiding creating copies of them when creating RPC requests, and zeroing them when finished.

This is not a totally solvable problem (it gets much worse) but a best attempt should be made regardless.

I'm an applications dev and not a graphics designer :)

There's a lot of possible improvements that can be made to the XAML definitions, especially with regards to a common style. Currently, many colors and pixel offsets are hardcoded instead of defining new user control styles.

README and the WalletProcess code should be updated for the new options and remove the steps to checkout that dev branch.

Right now it only discusses stuff about recovery of the wallet, but this also needs to explain how it can easily lead to theft if the seed is compromised by a 3rd party. Some users seem confused and give out their seeds to others, perhaps not knowing that their entire wallet could be stolen by doing so.

Ideally the credentials for btcd should be saved and the connection dialog only shown if the saved credentials are invalid.

I find popup message boxes to be distracting and they don't fit in visually with the rest of the application (such as the dialog banners used for prompts). They're very easy to use and pretty great for development, but eventually I want them all gone.

The rename account dialog does not disable the rename button and provide details on why it's disabled for invalid entries.

There are at least two cases of invalid input:

• Specifying the existing name of the account being renamed
• Specifying an existing name of any account

Under these scenarios the rename dialog lets you press the rename button repeatedly and nothing happens.

I would suggest a label be added to the dialog which is displayed on conditions which prevent entry and for the rename button to be disabled in said cases. So, for example, entering the same name as the existing account would disable the rename button and show a label "An account already exists with the specified name."

At the moment, dust outputs are not checked for when constructing a new transaction. At the very least, the send should fail. A nicer implementation would show an error next to each dust output and prevent the transaction from ever being constructed.

Once #43 is done, the user could set a higher transaction fee, which could change the dust threshold. This is less surprising than modifying any fee estimation logic to use a higher relay fee just to accommodate a dust output.