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

fix(wallet): add default value for wait stage #4089

Merged
merged 3 commits into from
May 16, 2022

Conversation

mrnaveira
Copy link
Contributor

Description

Setting the default value of the wallet's config option command_send_wait_stage as Broadcast.

Motivation and Context

The wallet in command mode is failing with the default config, due to empty default value for command_send_wait_stage. But the value should be Broadcast by default as specified in all the config.toml files.

How Has This Been Tested?

  • All unit and integration test pass.
  • The wallet works in command mode (before the change it didn't).

@@ -93,7 +93,7 @@ impl Default for WalletConfig {
password: None,
contacts_auto_ping_interval: Duration::from_secs(30),
contacts_online_ping_window: 30,
command_send_wait_stage: String::new(),
command_send_wait_stage: "Broadcast".to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a string is this something that makes sense to be an enum. Do we have a set of common values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get your point. I opted for the simplest solution (string) as:

  • The enum already exists (TransactionStage) and is defined in the tari_console_wallet application. I didn't use it because it doesn't feel right to include a dependency from an application (tari_console_wallet) into the base layer wallet.
  • Another option could be to move the whole enum definition into the base layer wallet...this could do but again I feel this may be breaking a bit the separation of concerns between the base layer and the applications.

I'm open to suggestions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted for the simplest solution (string)

I get this. Now that I know it does exist somewhere I understand the usage.

Another option could be to move the whole enum definition into the base layer wallet...this could do but again I feel this may be breaking a bit the separation of concerns between the base layer and the applications.

For the sake of conversation though, this doesn't feel wrong. We're introducing the concept into the base layer wallet via the string anyway. The base layer wallet is a dependency of the console wallet. If we moved the enum up a layer we haven't created or broken any new separation of concerns anymore than adding the string does.

Again- just for conversations sake. No change request required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The base layer wallet is a dependency of the console wallet. If we moved the enum up a layer we haven't created or broken any new separation of concerns anymore than adding the string does.

You convinced me ;)
Makes sense, I will do that

Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not really happy about the console wallet config settings being in the WalletConfig struct. It's not 100% correct, but I put them there as a first step. They should actually come out and be put into a local config struct for the console wallet. Given that, this PR is correct. Happy to merge

utACK

@aviator-app aviator-app bot merged commit 810d9ff into tari-project:development May 16, 2022
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

Successfully merging this pull request may close these issues.

5 participants