Skip to content

fix(cat-gateway): Fix cip36 endpoint #2124

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

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

fix(cat-gateway): Fix cip36 endpoint #2124

wants to merge 13 commits into from

Conversation

Mr-Leshiy
Copy link
Contributor

@Mr-Leshiy Mr-Leshiy commented Mar 31, 2025

Description

Based on #1986

  • Fixed cardano/cip36 endpoint

…sation (#2100)

* update cardano sync code

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* fix fmt

* change schema version

* wip

* wip

* wip

* wip
@Mr-Leshiy Mr-Leshiy changed the base branch from main to fix/cip36-assets-indexing March 31, 2025 20:50
Copy link
Contributor

github-actions bot commented Mar 31, 2025

Test Report | ${\color{lightgreen}Pass: 670/675}$ | ${\color{red}Fail: 0/675}$ |

stevenj and others added 4 commits April 1, 2025 11:03
* fix assets endpoint

* wip

* wip

* wip

* wip

* wip

* fix spelling

* Update catalyst-gateway/bin/src/service/common/objects/cardano/stake_info.rs

Co-authored-by: Steven Johnson <[email protected]>

* wip

---------

Co-authored-by: Steven Johnson <[email protected]>
@Mr-Leshiy Mr-Leshiy marked this pull request as ready for review April 1, 2025 09:17
@Mr-Leshiy Mr-Leshiy added the review me PR is ready for review label Apr 1, 2025
@Mr-Leshiy Mr-Leshiy moved this from New to 👀 In review in Catalyst Apr 1, 2025
Copy link
Collaborator

@stevenj stevenj left a comment

Choose a reason for hiding this comment

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

Logic in get_all_valid_registrations terminates early when there could be valid registrations, not desired behavior and not what it used to do.

// This should NOT happen, valid registrations should be infallible.
// If it happens, there is an indexing issue.
continue;
anyhow::bail!("Corrupt valid registration, invalid stake_pub_key {err}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we bailing here, and not just ignoring the corrupt registration? It may not be the latest, and if its not, it doesn't matter that its corrupt...
This causes us to fail to report otherwise valid registrations because there was a single error.

// This should NOT happen, valid registrations should be infallible.
// If it happens, there is an indexing issue.
continue;
anyhow::bail!("Corrupt invalid registration {err}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we bailing, and not just ignoring a registration which may not be the latest? This prevents us finding valid registrations because something was wrong.

} else {
continue;
let Ok(raw_nonce) = u64::try_from(row.raw_nonce) else {
anyhow::bail!("Corrupt valid registration, cannot decode raw_nonce");
Copy link
Collaborator

Choose a reason for hiding this comment

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

same, why are we bailing when there may be valid registrations yet to process?

regs_stream.map_err(Into::<anyhow::Error>::into).try_fold(Vec::new(), |mut regs, row|
async move {
let Ok(nonce) = u64::try_from(row.nonce) else {
anyhow::bail!("Corrupt valid registration, cannot decode nonce");
Copy link
Collaborator

Choose a reason for hiding this comment

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

why bail when there may be valid registrations we can actually process following it.

err, stake_pub_key
);
continue;
anyhow::bail!("Corrupt valid registration, invalid payment_address {err}\n Stake pub key:{stake_pub_key:?}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

why bail?

err, stake_pub_key
);
continue;
anyhow::bail!("Corrupt valid registration, invalid vote_pub_key {err}\n Stake pub key:{stake_pub_key:?}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

why bail?

@github-project-automation github-project-automation bot moved this from 👀 In review to 🔖 Ready in Catalyst Apr 1, 2025
@stevenj stevenj marked this pull request as draft April 1, 2025 13:27
@stevenj stevenj added the do not merge yet PR is not ready to be merged yet label Apr 1, 2025
@Mr-Leshiy Mr-Leshiy added draft Draft and removed review me PR is ready for review labels Apr 8, 2025
Base automatically changed from fix/cip36-assets-indexing to main April 17, 2025 15:06
@github-actions github-actions bot deleted the fix/cip36 branch May 19, 2025 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet PR is not ready to be merged yet draft Draft
Projects
Status: 🔖 Ready
Development

Successfully merging this pull request may close these issues.

2 participants