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

wrapped OUSD API #206

Closed
wants to merge 4 commits into from
Closed

wrapped OUSD API #206

wants to merge 4 commits into from

Conversation

rolandpo
Copy link
Contributor

@rolandpo rolandpo commented May 12, 2022

Adding wrapped OUSD transaction history functionality along with new API URL to be used with wOUSD history tab in the dapp

Connected PR: OriginProtocol/origin-dollar#995

@@ -125,6 +126,36 @@ def maybe_store_transfer_record(log, block):

return transfer

def maybe_store_wrap_transfer_record(log, block):
Copy link
Member

Choose a reason for hiding this comment

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

Pretty cool that you are also storing WOUSD transafers. This will surely be useful for other use cases

Copy link
Member

@sparrowDom sparrowDom left a comment

Choose a reason for hiding this comment

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

Great work on this one @rolandpo. Left some suggestions on how to improve code maintainability.

@@ -755,6 +758,114 @@ def process_transaction(transaction):

return analyzed_transactions

def ensure_analyzed_wrap_transactions(from_block, to_block, start_time, end_time, account='all'):
Copy link
Member

Choose a reason for hiding this comment

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

Huge amount of code seems to overlap between this function and ensure_analyzed_transactions. I think it would make sense to create a new function that has an extra bool is_ousd:

__ensure_transaction_analysis(from_block, to_block, start_time, end_time, account='all', mode):
    is_ousd = mode === 'ousd'
    .... (rest of code)

and both functions can call the newly created function:

def ensure_analyzed_transactions(from_block, to_block, start_time, end_time, account='all'):
    __ensure_transaction_analysis(from_block, to_block, start_time, end_time, account, 'ousd'

def ensure_analyzed_wrap_transactions(from_block, to_block, start_time, end_time, account='all'):
    __ensure_transaction_analysis(from_block, to_block, start_time, end_time, account, 'wousd'

This way you can reduce the duplicated code and increase maintainability -> There is less overall code to maintain and when you make a change in one of the functions you don't need to keep in mind that you need to make change in the other one that has duplicated code as well

@rolandpo
Copy link
Contributor Author

think that's sorted now

Copy link
Member

@sparrowDom sparrowDom left a comment

Choose a reason for hiding this comment

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

Added some comments on how you can additionally refactor the function. Do reach out if my comments need additional pointers!

@@ -679,54 +685,108 @@ def process_transaction(transaction):
sent_eth = transaction.data['value'] != '0x0'
transfer_ousd_out = False
transfer_ousd_in = False
transfer_wousd_out = False
Copy link
Member

Choose a reason for hiding this comment

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

this can further be reduced in size and complexity. I would not introduce new booleans here. Rather make the existing ones more general:
transfer_ousd_out -> transfer_token_out where token can be OUSD / WOUSD
transfer_ousd_in -> transfer_token_in where token can be OUSD / WOUSD
...

transfer_coin_out = False
transfer_coin_in = False
ousd_transfer_from = None
ousd_transfer_to = None
ousd_transfer_amount = None
wousd_transfer_from = None
wousd_transfer_to = None
wousd_transfer_amount = None
transfer_log_count = 0

for log in logs:
if log.topic_0 == TRANSFER:
transfer_log_count += 1
is_ousd_token = log.address == '0x2a8e1e676ec238d8a992307b495b45b3feaa5e86'
Copy link
Member

Choose a reason for hiding this comment

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

this would become then:

is_token = is_ousd ? log.address == '0x2a8e...' : log.address == '0xd2af...'

transfer_ousd_in = True
else:
transfer_coin_in = True
if is_ousd:
Copy link
Member

Choose a reason for hiding this comment

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

Most of additional boolean branching below would also be reduced:

if is_token:
    token_transfer_from = from_address
....

if is_ousd:
if transfer_log_count > 0:
if transfer_ousd_in:
classification = 'transfer_in'
Copy link
Member

Choose a reason for hiding this comment

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

This line would become:

classification = is_ousd ? 'transfer_in' : 'transfer_in_wousd'

wrap_receive_wousd = transfer_wousd_in and transfer_coin_out
wrap_send_wousd = transfer_wousd_out and transfer_coin_in

if is_ousd:
Copy link
Member

Choose a reason for hiding this comment

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

no need for this if statement I think

@rolandpo
Copy link
Contributor Author

makes sense for sure, that should be fixed now

Copy link
Member

@sparrowDom sparrowDom left a comment

Choose a reason for hiding this comment

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

lgtm
nvm commented on the wrong issue

@rolandpo
Copy link
Contributor Author

replaced by #362

@rolandpo rolandpo closed this Aug 29, 2023
@shahthepro shahthepro deleted the rolandpo/wousd branch September 21, 2023 16:37
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.

2 participants