-
Notifications
You must be signed in to change notification settings - Fork 19
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
Feature: upgrade balance endpoint #471
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just add tests.
tests/conftest.py
Outdated
with session_factory() as session: | ||
session.add(balance) | ||
session.commit() | ||
insert_volume_refs(session, fixture_instance_message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why you insert the volumes in this fixture, as it has nothing to do with the balance. I expect 3 fixtures:
instance_message
instance_message_with_volumes_in_db
(depends on the first one)user_balance
(depends on nothing).
tests/api/test_balance.py
Outdated
assert response.status == 200, await response.text() | ||
data = await response.json() | ||
assert data["balance"] == 22192 | ||
assert data["locked_amount"] == 6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come this value is so low?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cause i did only commit the volume and not instances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, what's the point of importing the instance fixture if you don't use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now it's implement too, just miss to add
Co-authored-by: Olivier Desenfans <[email protected]>
Problem:
Currently, the Balance API Endpoint only returns the total balance for an address. And user (also frontend) as no easy way to get number of locked token.
On :
/api/v0/addresses/{address}/balance
Solution:
Enhance the Balance API Endpoint to include the "locked_amount" field in the response, indicating the amount of tokens in use for a specific address.
Example: