-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fix token balance check and UDC allowance minting #275
Conversation
@property | ||
def balance(self): | ||
"""Proxy the balance call to the UDTC.""" | ||
return self.token_proxy.contract.functions.balanceOf(self.ud_token_address).call() |
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'm not 100% convinced on this fix.
Here is a test script. I just wanted to make sure that a subclass could use super()
and the code of the super class would still use the subclass properties:
class A:
@property
def balance(self):
return 1
def mint(self):
return self.balance
class B(A):
def mint(self):
return super().mint()
@property
def balance(self):
return 2
a, b = A(), B()
print(a.balance, a.mint(), b.balance, b.mint())
The above will print 1 1 2 2
, meaning that it does.
This happens on our code here, the call self.udc.mint(our_address)
will end up using the balance define here, which is the balance of ud_token_address
. If I understood this correctly, the formula self.config.token.max_funding - balance
will eventually only negative values (assuming there is a scenario that does not use the complete UDC deposit).
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.
super class would still use the subclass properties
If that wasn't the case OO would be pretty broken in Python ;)
With the additional changes it should now be correct (since the balance of the SP account is used to deposit
into the separate Raiden nodes).
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.
This should fix #238
Codecov seems to be stuck. Since the tests are green and it's approved I'll force merge this. |
Yeah! |
Fixes: #238