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

Improve registry script usability #890

Merged
merged 10 commits into from
Nov 23, 2020

Conversation

karlb
Copy link
Contributor

@karlb karlb commented Nov 20, 2020

A large collection of UI improvements for the service registration. Highlights:

  • info command to show past deposits
  • extend command to prolong registration
  • no need to remember your deposit address, withdraw will find it for you

Closes #768 and #490.

@auto-assign auto-assign bot requested a review from konradkonrad November 20, 2020 14:54
@codecov
Copy link

codecov bot commented Nov 20, 2020

Codecov Report

Merging #890 (8954566) into master (7ea7397) will increase coverage by 0.05%.
The diff coverage is 78.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #890      +/-   ##
==========================================
+ Coverage   90.66%   90.72%   +0.05%     
==========================================
  Files          41       41              
  Lines        2893     2986      +93     
  Branches      356      373      +17     
==========================================
+ Hits         2623     2709      +86     
- Misses        192      195       +3     
- Partials       78       82       +4     
Impacted Files Coverage Δ
src/raiden_libs/service_registry.py 81.77% <78.78%> (+7.65%) ⬆️
src/pathfinding_service/api.py 94.96% <0.00%> (-0.72%) ⬇️
src/raiden_libs/blockchain.py 86.60% <0.00%> (+0.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ea7397...8954566. Read the comment docs.

Comment on lines 232 to 234
if web3.eth.chainId == 1:
click.secho("You don't have sufficient tokens", err=True)
sys.exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the flow here:
a) where is the check that the user does not have "sufficient tokens"?
b) why don't we show the token amount the user has?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are completely right. I moved those lines around a few times too often and apparently mixed up the conditions the last time. Should be better, now.

deposits = [d for d in deposits if not d["withdrawn"]]
if not deposits:
click.echo("No deposits found!", err=True)
sys.exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not finding withdrawable deposits does not necessarily constitute an error (exit > 0) to me. Does it to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If withdraw is called, I would expect a withdraw to happen. It feels similar to an rm with a filename that does not exist to me. I do see your point, but prefer the current return code.

Copy link
Contributor

@konradkonrad konradkonrad left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

This makes withdrawing a lot easier for the service operator.
Example output:

```
 * block 2728455, amount: 1991640781850122022103 REI (1991.64 RDN),
 WITHDRAWN
 * block 3781847, amount: 10227217067851600044335 REI (10227.22 RDN),
  increased validity till 2021-05-18 15:34:32
```
I could not find a good document about our auction format. Otherwise, I
would have linked to that.
@karlb karlb force-pushed the improve-registry-script branch from f2f5c3b to cf714d4 Compare November 23, 2020 14:16
@karlb karlb force-pushed the improve-registry-script branch from cf714d4 to 8954566 Compare November 23, 2020 14:22
@karlb karlb merged commit d1d126e into raiden-network:master Nov 23, 2020
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.

Service registration script needs UX improvements
2 participants