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

fix(cat-gateway): Fix native asset indexing to be more flexible #1150

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

fix(cat-gateway): Fix native asset indexing to be more flexible #1150

wants to merge 16 commits into from

Conversation

apskhem
Copy link
Collaborator

@apskhem apskhem commented Nov 6, 2024 •
edited
Loading

Description

  • Renamed the schema.
  • Renamed the response object.
  • Renamed the database mapping object.
  • Fixed asset map to handle list of txos.

Related Issue(s)

Closes #1121

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

@apskhem apskhem added the bug Something isn't working label Nov 6, 2024
@apskhem apskhem self-assigned this Nov 6, 2024
@apskhem apskhem linked an issue Nov 6, 2024 that may be closed by this pull request
[Bug]: Native Asset indexing and endpoint response has issues that need correcting. #1121
Open
2 tasks
@apskhem apskhem marked this pull request as draft November 6, 2024 13:41
@apskhem apskhem added the review me PR is ready for review label Nov 6, 2024
@apskhem apskhem marked this pull request as ready for review November 6, 2024 13:51
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 6, 2024

✅ Test Report | ${\color{lightgreen}Pass: 291/291}$ | ${\color{red}Fail: 0/291}$ |

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 6, 2024

✅ Test Report | ${\color{lightgreen}Pass: 320/320}$ | ${\color{red}Fail: 0/320}$ |

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 6, 2024

✅ Test Report | ${\color{lightgreen}Pass: 320/320}$ | ${\color{red}Fail: 0/320}$ |

@stevenj stevenj marked this pull request as draft November 7, 2024 09:30
@stevenj stevenj added this to the M4: Voting & Delegation milestone Nov 7, 2024
stevenj
stevenj requested changes Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@stevenj stevenj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

See comments, these need to be corrected and then I will conduct another review.

catalyst-gateway/bin/src/db/index/schema/cql/txo_assets_by_stake_table.cql Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/db/index/schema/cql/txo_assets_by_stake_table.cql Outdated Show resolved Hide resolved
catalyst-gateway/bin/src/db/index/schema/cql/unstaked_txo_assets_by_txn_hash.cql Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 7, 2024

✅ Test Report | ${\color{lightgreen}Pass: 291/291}$ | ${\color{red}Fail: 0/291}$ |

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 7, 2024

✅ Test Report | ${\color{lightgreen}Pass: 320/320}$ | ${\color{red}Fail: 0/320}$ |

@apskhem apskhem requested a review from stevenj November 7, 2024 15:39
@apskhem apskhem marked this pull request as ready for review November 8, 2024 13:43
@apskhem apskhem marked this pull request as draft November 11, 2024 12:33
@apskhem apskhem marked this pull request as ready for review November 11, 2024 12:53
@github-actions GitHub Actions
Copy link
Contributor

✅ Test Report | ${\color{lightgreen}Pass: 320/320}$ | ${\color{red}Fail: 0/320}$ |

@github-actions GitHub Actions
Copy link
Contributor

✅ Test Report | ${\color{lightgreen}Pass: 320/320}$ | ${\color{red}Fail: 0/320}$ |

Mr-Leshiy
Mr-Leshiy reviewed Nov 13, 2024
View reviewed changes
Copy link
Contributor

@Mr-Leshiy Mr-Leshiy left a comment

Choose a reason for hiding this comment

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

From the original issue it is also telling to the change the asset value type to i128, which I dont see in this PR.
Is it really needed, or its something we dont want to do ?

catalyst-gateway/bin/src/service/api/cardano/staking/assets_get.rs
amount: num_bigint::BigInt::ZERO,
});
entry.amount += row.value;
.or_insert_with(Vec::new);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite get it this change.
Could you elaborate what we are doing here actually ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main point is to make the value of the map to be able to store multiple assets for a policy id (previously one policy id for one asset only).

@github-actions GitHub Actions
Copy link
Contributor

✅ Test Report | ${\color{lightgreen}Pass: 29/29}$ | ${\color{red}Fail: 0/29}$ |

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mr-Leshiy Mr-Leshiy Mr-Leshiy left review comments

@FelipeRosa FelipeRosa Awaiting requested review from FelipeRosa

@cong-or cong-or Awaiting requested review from cong-or cong-or is a code owner

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

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

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

Requested changes must be addressed to merge this pull request.

Assignees

@apskhem apskhem

Labels
bug Something isn't working review me PR is ready for review
Projects
Catalyst
Status: 👀 In review
Milestone
M4: Voting & Delegation
Development

Successfully merging this pull request may close these issues.

[Bug]: Native Asset indexing and endpoint response has issues that need correcting.
4 participants
@apskhem @stevenj @Mr-Leshiy @minikin