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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
May 16, 2022
Merged

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e830486
add default value for wait stage
mrnaveira May 11, 2022
1ed9a15
move the enum to the base layer
mrnaveira May 11, 2022
925e344
Merge branch 'development' into fix-wallet-config
aviator-app[bot] May 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion base_layer/wallet/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

command_send_wait_timeout: Duration::from_secs(300),
notify_file: None,
grpc_address: None,
Expand Down