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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 3, 2019
Merged

Fix token balance check and UDC allowance minting #275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d35c7e6
Fix token balance check
ulope Sep 2, 2019
f64d189
Properly calculate UDC token minting amount
ulope Sep 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions scenario_player/runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,12 +308,16 @@ def _initialize_udc(
self.udc = UserDepositContract(self, udc_ctr, ud_token_ctr)

should_deposit_ud_token = udc_enabled and udc_settings.token["deposit"]
allowance_tx = self.udc.update_allowance()
allowance_tx, required_allowance = self.udc.update_allowance()
if allowance_tx:
ud_token_tx.add(allowance_tx)
if should_deposit_ud_token:

tx = self.udc.mint(our_address)
tx = self.udc.mint(
our_address,
required_balance=required_allowance,
max_fund_amount=required_allowance * 2,
)
if tx:
ud_token_tx.add(tx)

Expand Down
10 changes: 6 additions & 4 deletions scenario_player/scenario.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,10 @@ def validate(self):
If token.min_balance < settings.services.udc.token.max_funding
"""

# FIXME: This check seems to make no sense. The scenario token should be independent of the
# UDC token @nlsdfnbch
# Check that the amount of minted tokens is >= than the amount of deposited tokens
try:
assert self.token.min_balance >= self.settings.services.udc.token.max_funding
except AssertionError:
raise InsufficientMintingAmount
# try:
# assert self.token.min_balance >= self.settings.services.udc.token.max_funding
# except AssertionError:
# raise InsufficientMintingAmount
54 changes: 40 additions & 14 deletions scenario_player/utils/token.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,15 +61,18 @@ def transact(self, action: str, parameters: dict) -> str:
log.info(f"'{action}' call succeeded", tx_hash=tx_hash)
return decode_hex(tx_hash)

def mint(self, target_address, **kwargs) -> Union[str, None]:
def mint(
self, target_address, required_balance=None, max_fund_amount=None, **kwargs
) -> Union[str, None]:
"""Mint new tokens for the given `target_address`.

The amount of tokens depends on the scenario yaml's settings, and defaults to
:attr:`.DEFAULT_TOKEN_BALANCE_MIN` and :attr:`.DEFAULT_TOKEN_BALANCE_FUND`
if those settings are absent.
"""
balance = self.balance
required_balance = self.config.token.min_balance
if required_balance is None:
required_balance = self.config.token.min_balance
log.debug(
"Checking necessity of mint request",
required_balance=required_balance,
Expand All @@ -79,7 +82,10 @@ def mint(self, target_address, **kwargs) -> Union[str, None]:
log.debug("Mint call not required - sufficient funds")
return

mint_amount = self.config.token.max_funding - balance
if max_fund_amount is None:
max_fund_amount = self.config.token.max_funding

mint_amount = max_fund_amount - balance
log.debug("Minting required - insufficient funds.")
params = {"amount": mint_amount, "target_address": target_address}
params.update(kwargs)
Expand All @@ -102,6 +108,7 @@ def __init__(self, scenario_runner, data_path: pathlib.Path):
self._token_file = data_path.joinpath("token.info")
self.contract_data = {}
self.deployment_receipt = None
self.contract_proxy = None

@property
def name(self) -> str:
Expand Down Expand Up @@ -159,7 +166,7 @@ def balance(self) -> float:
It is an error to access this property before the token is deployed.
"""
if self.deployed:
return super(Token, self).balance
return self.contract_proxy.contract.functions.balanceOf(self.address).call()
else:
raise TokenNotDeployed

Expand Down Expand Up @@ -258,18 +265,21 @@ def use_existing(self) -> Tuple[str, int]:
f"Cannot reuse token - address {address} has no code stored!"
) from e

# Fetch the token's contract_proxy data.
contract_proxy = self._local_contract_manager.get_contract(contract_name)
# Fetch the token's contract_info data.
contract_info = self._local_contract_manager.get_contract(contract_name)

self.contract_data = {"token_contract": address, "name": contract_proxy.name}
self.contract_data = {"token_contract": address, "name": contract_name}
self.contract_proxy = self._local_rpc_client.new_contract_proxy(
contract_info["abi"], address
)
self.deployment_receipt = {"blockNum": block}
checksummed_address = to_checksum_address(address)

log.debug(
"Reusing token",
address=checksummed_address,
name=contract_proxy.name,
symbol=contract_proxy.symbol,
name=contract_name,
symbol=self.contract_proxy.contract.functions.symbol().call(),
)
return checksummed_address, block

Expand Down Expand Up @@ -308,8 +318,13 @@ def deploy_new(self) -> Tuple[str, int]:
resp_data["deployment_block"],
)

contract_info = self._local_contract_manager.get_contract("CustomToken")

# Make deployment address and block available to address/deployment_block properties.
self.contract_data = token_contract_data
self.contract_proxy = self._local_rpc_client.new_contract_proxy(
contract_info["abi"], token_contract_data["address"]
)
self.deployment_receipt = {"blockNumber": deployment_block}

if self.config.token.reuse_token:
Expand Down Expand Up @@ -349,6 +364,11 @@ def allowance(self):
self._local_rpc_client.address, self.address
).call()

@property
def balance(self):
"""Proxy the balance call to the UDTC."""
return self.token_proxy.contract.functions.balanceOf(self.ud_token_address).call()
Copy link
Contributor

@hackaugusto hackaugusto Sep 2, 2019 •
edited
Loading

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).

Copy link
Contributor Author

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).


def effective_balance(self, at_target):
"""Get the effective balance of the target address."""
return self.contract_proxy.contract.functions.effectiveBalance(at_target).call()
Expand All @@ -357,13 +377,19 @@ def total_deposit(self, at_target):
""""Get the so far deposted amount"""
return self.contract_proxy.contract.functions.total_deposit(at_target).call()

def mint(self, target_address) -> Union[str, None]:
def mint(
self, target_address, required_balance=None, max_fund_amount=None, **kwargs
) -> Union[str, None]:
"""The mint function isn't present on the UDC, pass the UDTC address instead."""
return super(UserDepositContract, self).mint(
target_address, contract_address=self.ud_token_address
return super().mint(
target_address,
required_balance=required_balance,
max_fund_amount=max_fund_amount,
contract_address=self.ud_token_address,
**kwargs,
)

def update_allowance(self) -> Union[str, None]:
def update_allowance(self) -> Union[Tuple[str, int], None]:
"""Update the UD Token Contract allowance depending on the number of configured nodes.

If the UD Token Contract's allowance is sufficient, this is a no-op.
Expand All @@ -389,7 +415,7 @@ def update_allowance(self) -> Union[str, None]:
"target_address": self.checksum_address,
"contract_address": self.ud_token_address,
}
return self.transact("allowance", params)
return self.transact("allowance", params), required_allowance

def deposit(self, target_address) -> Union[str, None]:
"""Make a deposit at the given `target_address`.
Expand Down