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

Ticker check #8

Merged
merged 7 commits into from
Jan 30, 2023
Merged

Ticker check #8

merged 7 commits into from
Jan 30, 2023

Conversation

arkanoider
Copy link
Collaborator

While starting to work on #3 I tried to implement ticker check, now:

  • Ticker requested from cli is uppercased and checked with a list created from the JSON file.

Maybe it's not the best rust implementation, but is working. Another small update could be a command like:

mostro-cli list-currencies

to get a print on std output of all the tickers...also if it's a bit long.

Let me know!!

@grunch
Copy link
Member

grunch commented Jan 29, 2023

Hi @arkanoider !

Thanks, I see you uppercased the currency, the currency list can be useful, if the user use one currency we know, we can use some stuff related from that list like the emoji for example, but as i commented here #2 (comment) we shouldn't limit users to use our fiat currency list because users could use any other currency they like, it can be a regional currency which we don't know it exists or even a shitcoin.

This should try to find if there is orders for the currency with code xxx and not give an currency not available error:

mostro-cli list-orders -c xxx

@arkanoider
Copy link
Collaborator Author

Hi @grunch got it, we can avoid the error message if it's not on the list and only check against the result from the relays. We can avoid using the list at all at this point except for some cosmetics. Will work on it!

…e only if there is a match with selected curency
@arkanoider
Copy link
Collaborator Author

Hi @grunch did ticker check more "free" users now can select any currency ticker and it will be upcased and checked against relay result. In case of no match with result an error table like this is printed out. I think it's more like what you were thinkin about. We can use anyway later the parse of "famous" currencies to do some cosmetics.

Table error like:
Istantanea_2023-01-29_23-33-53

Copy link
Member

@grunch grunch left a comment

Choose a reason for hiding this comment

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

This looks great, there is some few syntax things to fix before merge.

A good way of check your own code before commit is doing cargo fmt and cargo clippy, it helps a lot.

src/main.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated
@@ -33,6 +37,24 @@ async fn main() -> Result<()> {
}) => {
let mostro_key = XOnlyPublicKey::from_bech32(pubkey)?;

//Commented for later use maybe...
Copy link
Member

Choose a reason for hiding this comment

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

let's remove the code we are not using

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/util.rs Outdated
//Table rows
let mut rows: Vec<Row> = Vec::new();

if orderstable.is_empty(){
Copy link
Member

Choose a reason for hiding this comment

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

orderstable => orders_table

src/util.rs Outdated Show resolved Hide resolved
@arkanoider
Copy link
Collaborator Author

Hi @grunch should have fixed all Rust basic rules. Let me know.

Check this :

https://github.com/arkanoider/mostro-cli/blob/8eb1c56c5b188db8922acaab530f805b1fc044d3/src/main.rs#L22

Moved after check on verbose flag to correctly initialize env var in the case it's activated.

@grunch
Copy link
Member

grunch commented Jan 30, 2023

Looks great, thanks @arkanoider

@grunch grunch merged commit 31dc81b into MostroP2P:main Jan 30, 2023
@arkanoider
Copy link
Collaborator Author

Amazing! Let's continue on #3

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.

2 participants