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

Jump to bottom

feat(apps/whale-api): Add consortium transaction history endpoint #1868

Open
wants to merge 65 commits into
base: main
Choose a base branch
from
Open

feat(apps/whale-api): Add consortium transaction history endpoint #1868

wants to merge 65 commits into from

Conversation

helloscoopa
Copy link
Contributor

@helloscoopa helloscoopa commented Nov 22, 2022 •
edited
Loading

What this PR does / why we need it:

Adds transaction history endpoint to be used in upcoming consortium tab on defiscan.live

Initially, this will return transactions among consortium members related to token mints and burns.

Additional comments?:

Merge #1923 before merging this PR.

infinia-yzl
infinia-yzl suggested changes Nov 23, 2022
View reviewed changes
Copy link
Contributor

@infinia-yzl infinia-yzl left a comment

Choose a reason for hiding this comment

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

  • What happens if it searches by TxId that is not part of the consortium or not a mint/burn transaction?
  • Are there transactions other than mint or burn in the test container? If not, then can we add some noise to the data to check if it searches correctly?

apps/whale-api/src/module.api/consortium.controller.ts Outdated Show resolved Hide resolved
apps/whale-api/src/module.api/consortium.controller.spec.ts Outdated Show resolved Hide resolved
apps/whale-api/src/module.api/consortium.controller.spec.ts Outdated Show resolved Hide resolved
@helloscoopa
Copy link
Contributor Author

  • What happens if it searches by TxId that is not part of the consortium or not a mint/burn transaction?
  • Are there transactions other than mint or burn in the test container? If not, then can we add some noise to the data to check if it searches correctly?

We already have some fund transfering txes in the setup which creates sent type transactions and also blockReward transactions, I guess we can count on those for this query -- however I've added one more test to specifically search for a type: sent txid to see if it return empty list. 👍🏼

infinia-yzl
infinia-yzl previously approved these changes Nov 24, 2022
View reviewed changes
Copy link
Contributor

@infinia-yzl infinia-yzl left a comment

Choose a reason for hiding this comment

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

LGTM

fuxingloh pushed a commit that referenced this pull request Jan 19, 2023
@helloscoopa @infinia-yzl
feat(jellyfish-api-core): Add index based pagination support to `list…
3d0e633 
…accounthistory` RPC (#1923)

<!--  Thanks for sending a pull request! -->

#### What this PR does / why we need it:

**Requirement:** Upcoming consortium transaction history page on
[defiscan.live](https://defiscan.live/) requires a way to paginate the
list of transactions among consortium members.

**Issue:** Currently we only have `blockHeight` and `depth` based
pagination/cursor which we're unable to use to address the requirement
as it only cursor through blocks but not transactions., More info
[here](#1868 (comment)).

**Solution:** 
- Add index based pagination support.
- Add multi transaction types filter support.
- Add multi address filter support.

Huge thanks to @shohamc1 for supporting me on defich/ain side, refer
[defich/ain PR](DeFiCh/ain#1643)

Signed-off-by: Dilshan Madushanka <[email protected]>
Co-authored-by: Isaac Yong <[email protected]>
Co-authored-by: Kven Ho <[email protected]>
Base automatically changed from dilshan/listaccounthistory-pagination to main January 19, 2023 08:37
@fuxingloh fuxingloh dismissed stale reviews from marktanrj, delphk, imkven, and infinia-yzl via 2388960 January 19, 2023 08:37
marktanrj
marktanrj previously approved these changes Jan 19, 2023
View reviewed changes
infinia-yzl
infinia-yzl previously approved these changes Jan 19, 2023
View reviewed changes
@helloscoopa helloscoopa dismissed stale reviews from infinia-yzl and marktanrj via 19ee8f0 January 20, 2023 03:34
infinia-yzl
infinia-yzl previously approved these changes Jan 20, 2023
View reviewed changes
marktanrj
marktanrj previously approved these changes Jan 20, 2023
View reviewed changes
imkven
imkven previously approved these changes Jan 20, 2023
View reviewed changes
fuxingloh
fuxingloh suggested changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@fuxingloh fuxingloh left a comment

Choose a reason for hiding this comment

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

The use of this.rpcClient.account need to be properly designed/managed as they're expensive to call.

I spot 3 uses of this.rpcClient.account.*; to review it properly, we need quite a lot of time to understand the implications of it. I will check with @thedoublejay on the use cases; there might be an alternative.

apps/whale-api/src/module.api/consortium.service.ts
limit
})

const totalTxCount = await this.rpcClient.account.historyCount(memberAddresses, { txtypes: [DfTxType.BURN_TOKEN, DfTxType.MINT_TOKEN] })
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is a LevelDB:Scan all records operation, and very expensive to call. Can we avoid this or find alternatives?

Copy link
Contributor Author

@helloscoopa helloscoopa Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For each API call, there will be maximum 2 rpc calls eventhough we have 3 this.rpcClient.account.* usages, as the getSingleHistoryTransactionResponse terminates the flow.

About alternatives: I couldn't find find an alternative to get total count and transactions in terms of RPCs thus I used the combination of these historyCount and listAccountHistory. Though, Maybe I should take a look if I can do something with the indexer?!

apps/whale-api/src/module.api/consortium.service.ts Show resolved Hide resolved
apps/whale-api/src/module.api/consortium.controller.ts Outdated Show resolved Hide resolved
@helloscoopa helloscoopa dismissed stale reviews from imkven, marktanrj, and infinia-yzl via 9e171c7 January 25, 2023 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fuxingloh fuxingloh fuxingloh requested changes

@delphk delphk delphk left review comments

@marktanrj marktanrj marktanrj left review comments

@infinia-yzl infinia-yzl infinia-yzl left review comments

@imkven imkven imkven left review comments

@canonbrother canonbrother Awaiting requested review from canonbrother canonbrother is a code owner

@ivan-zynesis ivan-zynesis Awaiting requested review from ivan-zynesis ivan-zynesis is a code owner

@kodemon kodemon Awaiting requested review from kodemon

@eli-lim eli-lim Awaiting requested review from eli-lim

@pierregee pierregee Awaiting requested review from pierregee

@thedoublejay thedoublejay Awaiting requested review from thedoublejay thedoublejay is a code owner

@kyleleow kyleleow Awaiting requested review from kyleleow

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
apps/whale-api area/apps area/packages kind/feature New feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@helloscoopa @imkven @infinia-yzl @fuxingloh @pierregee @marktanrj @delphk @shohamc1