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

Hotfix LIT/DOT Holder VC #2795

Merged
merged 7 commits into from
Jun 11, 2024
Merged

Hotfix LIT/DOT Holder VC #2795

merged 7 commits into from
Jun 11, 2024

Conversation

Kailai-Wang
Copy link
Collaborator

@Kailai-Wang Kailai-Wang commented Jun 10, 2024

Context

It may fix other VCs incidentally too.

The root cause is we missed one place during the AbortStrategy refactoring, so it always returns false.

I also don't think we need metadata for query_system_label or check_achainable_label (btw what are the differences between these?), with metadata I could get response that consists of 10000 lines of JSON...

Additionally, I think total_transactions doesn't need metadata either

But please double-check it @higherordertech @zhouhuitian

Copy link

linear bot commented Jun 10, 2024

P-838 LIT/DOT Holder VC

@Kailai-Wang Kailai-Wang requested a review from outofxxx June 10, 2024 13:53
@Kailai-Wang Kailai-Wang self-assigned this Jun 10, 2024
@Kailai-Wang Kailai-Wang requested review from jonalvarezz, higherordertech and a team June 10, 2024 13:53
@Kailai-Wang
Copy link
Collaborator Author

Kailai-Wang commented Jun 10, 2024

This PR bumps the version too

The (parachain) runtime version is bumped retroactively - our tee-prod has 9181 already, which was upgraded manually when we reverted the NoEligibleIdentity PR which resulted in a different error type

@@ -111,7 +111,7 @@ pub fn request_achainable(
)
})?;

Ok(false)
Ok(result)
Copy link
Contributor

Choose a reason for hiding this comment

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

🙃

Copy link
Contributor

@higherordertech higherordertech Jun 11, 2024

Choose a reason for hiding this comment

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

Well spot, but this issue was not introduced in AbortStrategy refactoring PR (because the AbortStrategy PR doesn't try to alter any existing logic in general) but it actually this was already like this before the AbortStrategy PR: https://github.com/litentry/litentry-parachain/pull/2707/files#diff-c23f6a3018245418b1a8d1c1dc7187aeae2c92785b32294c437e46849be35aaeR114, i.e. the original implementation might have problems since day 1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see - thanks.

I'm wondering if it ever worked before - including our data provider test. cc @0xverin @BillyWooo

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor

@higherordertech higherordertech left a comment

Choose a reason for hiding this comment

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

LGTM

@Kailai-Wang Kailai-Wang enabled auto-merge (squash) June 11, 2024 13:28
@Kailai-Wang Kailai-Wang merged commit 1362036 into dev Jun 11, 2024
33 checks passed
@Kailai-Wang Kailai-Wang deleted the p-838-litdot-holder-vc branch June 11, 2024 17:13
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.

5 participants