Conversation
liorama
force-pushed
the
lior/ecosystem
branch
2 times, most recently
from
d337bbb to
6c9491d
Compare
|
LGTM, but why is travis failing? |
doodyparizada
force-pushed
the
lior/ecosystem
branch
from
348aceb to
1ac0f4f
Compare
src/app.py
Outdated
| raise MigrationErrors.AccountNotBurnedError(account_address) | ||
| logger.info(f'Verified that account {account_address} is burned') | ||
|
|
||
| account_data = get_account_data(account_address) |
There was a problem hiding this comment.
@liorama There is some duplication here, as now is_burned is also calling horizon for the account_data.
src/helpers.py
Outdated
| if not is_valid_address(account_address): | ||
| raise MigrationErrors.AddressInvalidError(account_address) | ||
|
|
||
| account_data = get_account_data(account_address) |
There was a problem hiding this comment.
@liorama @Ronserruya Didn't we want to also test that the trust limit has changed to the current kin balance? To make sure the user can't receive more KIN?
There was a problem hiding this comment.
Burning and setting the balance is 1 transaction so if you burn you always set the balance.
In any case it doesn't matter, once the account is burned we migrate it, it is not like he can set his limit now and ask again since he already burned his account
There was a problem hiding this comment.
If you just burn without setting the limit someone can use P2P to send your address KIN which will then be lost.
There was a problem hiding this comment.
Right, but I mean that the code on the client sends both ops in one tx, so its not like you can burn without setting the limit.
There was a problem hiding this comment.
@Ronserruya @doodyparizada a client without the KIN asset will fail to retrieve it's KIN balance and I assume an exception is thrown when this occurs. This will inevitable cause burning the wallet to fail because both ops are sent in a single transaction.
These condition may already be accounted (or required to be) for on android/ios client but I wanted to give my thoughts on it.
There was a problem hiding this comment.
@chancity When a client doesn't have the kin asset, he doesn't go through migration, the client just starts on the new blockchain
| def error_handle(exception: Exception): | ||
| # Log the exception and return an internal server error | ||
| logger.error(f'Unexpected exception: {str(exception)}') | ||
| logger.exception(f'Unexpected exception: {str(exception)}') |
There was a problem hiding this comment.
logger.exception adds the stacktrace of the last exception raised. What if some other exception is raised before getting to this method?
Wouldn't it be better to use
logger.exception(f'Unexpected exception: {str(exception)}', exc_info=exception)
To specifiy what exception you want the stack trace of?
There was a problem hiding this comment.
The error handler will be invoked on any exception that was thrown and not caught, I think this is the stacktrace I want to see.
optimize for zero balance + caching layer
allow 0 balance to migrate
use gevent
5 participants
No description provided.