gnosis / gnosis/dex-cli

Address all Leandro's feedback

Open
#39 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
5
Forks
3
PR merge metrics
No merged PRs in 30d

Description

I'll accumulate in this issue, all the good tips from Leandro.
Since there's MANY waterfall PRs and approved with nits and comments, I'll make this issue so I merge all and then address the suggested comments :)

# 1. Use var for price:
* [x] Done?

Nit: I would put the prices into their own variables and simplify the string building to something like:

```js
price = format_price(
calculate_price(
numerator=boughtVolume,
denominator=soldVolume,
decimals_numerator=buyTokenDecimals,
decimals_denominator=sellTokenDecimals
),
currency=buyTokenLabel
)
priceStr1 = f'{click.style(f' Avg. Traded Price {sellTokenLabel}/{buyTokenLabel}', fg=labelColor)}: {price1}'

...

tradePriceText = f'{priceStr1}\n{priceStr2}\n'
```
https://github.com/gnosis/dex-cli/pull/32#discussion_r394413888

# 2. Max sell volume, max receiving amount
* [ ] Done?

Would it be too much to also add the order size? Meaning, max buy/sell amounts, not just the matched amounts.

https://github.com/gnosis/dex-cli/pull/32#discussion_r394415191

# 3. Use f strings for formatting numbers
* [x] Done?

def format_integer(number):
return str(number) # TODO: Format better the numbers
return '{:,d}'.format(number)

Also in:
```
return f'{{:,.{decimals}f}}'.format(rounded_value).rstrip('0').rstrip('.')
```

Also in:
`return format_amount(percentage, decimals=2) + '%'`
https://github.com/gnosis/dex-cli/pull/34#discussion_r394467039

# 4. Unreachable code in `calculate_price`
* [x] Done?

https://github.com/gnosis/dex-cli/pull/32#discussion_r394425752

# 5 Simplify expression
* [x] Done?
https://github.com/gnosis/dex-cli/pull/32#discussion_r394421727

# 6. Improve sorting
* [ ] Done?
Can you enforce the option only accepts asc or desc?
What happens if something else is input?
https://github.com/gnosis/dex-cli/pull/33#discussion_r394462812

# 7. Use snake case for vars
* [x] Done?

Not comment from leandro, but I saw it now :)
`isUnlimitedAmount`

# 8. Improve for comprehension
Python magic 🌟
* [x] Done? This:
https://github.com/gnosis/dex-cli/pull/37#discussion_r394500885

* [x] Done? And this:
https://github.com/gnosis/dex-cli/pull/37#discussion_r394502145

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.