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

feat: token api #58

Closed
wants to merge 12 commits into from
Closed

feat: token api #58

wants to merge 12 commits into from

Conversation

justgigio
Copy link
Contributor

@justgigio justgigio linked an issue Aug 6, 2021 that may be closed by this pull request
4 tasks
Copy link
Member

@msbrogli msbrogli left a comment

Choose a reason for hiding this comment

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

I reviewed the design and part of the code. I wasn't sure how to review the whole code since I still had some questions regarding the design.

# Summary
[summary]: #summary

Token API will provide token information by requesting `hathor-wallet-service`.
Copy link
Member

Choose a reason for hiding this comment

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

I guess we don't have to explain the low level in the summary. Maybe explain where it will be used.

# Motivation
[motivation]: #motivation

We need to retrieve token data without requesting the full node. Wallet service holds those data, so, we'll use it for now.
Copy link
Member

Choose a reason for hiding this comment

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

The motivation is to replace the full node's API so the explorer can scale as much as we want without hitting the full node direclty.

# Drawbacks
[drawbacks]: #drawbacks

None.
Copy link
Member

Choose a reason for hiding this comment

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

It depends on the wallet service.

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

**Endpoints:**
Copy link
Member

Choose a reason for hiding this comment

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

I guess these endpoints belong to the guide level explanation.

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

**Endpoints:**
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use ## Endpoints.

) for token in self.tokens]
)
except Exception:
raise Exception('malformed_token_list')
Copy link
Member

Choose a reason for hiding this comment

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

Maybe raise a ValueError?

timestamp=transaction.timestamp
) for transaction in self.transactions]
)
except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

It feel dangerous to catch Exception. Why is that? It would capture ANY exception generated inside it (even if your code is broken).

) for transaction in self.transactions]
)
except Exception:
raise Exception('malformed_token_transaction_list')
Copy link
Member

Choose a reason for hiding this comment

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

Maybe raise ValueError?


@classmethod
def from_dict(cls, dikt: dict) -> 'HathorWalletServiceTokenResponse':
return from_dict(data_class=cls, data=dikt)
Copy link
Member

Choose a reason for hiding this comment

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

Does it validate type?


- We'll basically act as a proxy making requests to wallet service

# Reference-level explanation
Copy link
Member

Choose a reason for hiding this comment

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

Still in the low level design, how will it be deployed? Lambdas, right? I'm concerned that it can get really expensive if the Wallet Service's Token API hangs for any reason and we keep waiting for them. What would be a reasonable timeout here? Should we deploy it using k8s?

:return: Dict representations of TokenList
:rtype: dict
"""
return asdict(self)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like all of these to_dict and from_dict aren't really helping in any way. I'd be fine using asdict and from_dict directly, or at most making thin wrapper functions on a util module in case we want to customize some behavior. What do you think?

"""Response from hathor wallet service when requesting a token

:param id: id of token
:type id: str
Copy link
Member

Choose a reason for hiding this comment

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

Are we generating a docs with these comments? If we are, is it possible to make it use the type annotations instead of having to duplicate the information with :type? If we aren't I feel like we have no feedback if we're making a mistake or not, and I wouldn't bother with :type at all, but the descriptions are nice.

- Start Date: 2021-08-04
- RFC PR: (leave this empty)
- Hathor Issue: (leave this empty)
- Author: Giovane Costa <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

Since this isn't in the RFC repo, I think we should remove this, because these fields are mostly specific to the RFC repo. And maybe also drop the 0006- from the file name (I'm not sure if we have an RFC 0006 or not but it would certainly conflict with that).

@r4mmer r4mmer self-assigned this Aug 31, 2021
Base automatically changed from dev to main September 14, 2021 01:58
@r4mmer r4mmer self-requested a review as a code owner September 14, 2021 01:58
@lucas3003 lucas3003 deleted the feat/token-api branch April 26, 2022 00:46
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.

feat: token API
7 participants