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

Add RequestVcErrorDetail enum #2836

Merged
merged 14 commits into from
Jul 2, 2024

Conversation

grumpygreenguy
Copy link
Contributor

@grumpygreenguy grumpygreenguy commented Jun 24, 2024

Context

Partially implements P-609
Further refactoring of the API is needed, but deferred for now for timing constraints.

TODO

  • Pass CI

@grumpygreenguy grumpygreenguy added the C0-breaking Breaking change that will cause existing client to break label Jun 24, 2024
@grumpygreenguy grumpygreenguy self-assigned this Jun 24, 2024
Copy link

linear bot commented Jun 24, 2024

@grumpygreenguy grumpygreenguy requested review from Kailai-Wang, BillyWooo and felixfaisal and removed request for Kailai-Wang June 24, 2024 15:04
@@ -102,10 +103,33 @@ pub struct RequestVCResult {
pub pre_id_graph_hash: H256,
}

#[derive(Debug, Encode, Decode, Clone)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to incorporate the causing errors in the variants below, but ran into two issues:

  1. the respective (mostly 3rd party) error types don't implement the needed traits for derive to work here, and
  2. clippy complained that the type was "too big" for an Err variant, hence the Box in the AssertionBuildFailed variant :sadpanda:

@Kailai-Wang Kailai-Wang requested review from jonalvarezz and a team June 24, 2024 15:57
@grumpygreenguy grumpygreenguy enabled auto-merge (squash) June 25, 2024 07:46
Copy link
Collaborator

@BillyWooo BillyWooo left a comment

Choose a reason for hiding this comment

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

LGTM
is there any alignment with client-sdk?

@grumpygreenguy
Copy link
Contributor Author

LGTM is there any alignment with client-sdk?

We discussed it earlier with @jonalvarezz (although this PR goes for a smaller change in the end)

@Kailai-Wang Kailai-Wang requested a review from a team June 26, 2024 08:37
Copy link
Contributor

@jonalvarezz jonalvarezz left a comment

Choose a reason for hiding this comment

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

looking good to me!

are ts-tests updates coming in a follow-up pull requests? it needs update here, for instance

https://github.com/litentry/litentry-parachain/blob/a4f73b57b594ab070ebe8585b20d1f91630d334e/tee-worker/ts-tests/integration-tests/dr_vc.test.ts#L171

#[derive(Debug, Encode, Decode, Clone)]
pub struct RequestVcResultOrError {
pub payload: Vec<u8>,
pub is_error: bool,
pub result: Result<Vec<u8>, RequestVcErrorDetail>,
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! props for using the Result type

@grumpygreenguy
Copy link
Contributor Author

grumpygreenguy commented Jun 27, 2024

... oh no -_- CI is green, so that means that line isn't even exercised 😅

😄 i think it is – the value of the non-existent field is falsy, the condition doesn't meet

@grumpygreenguy
Copy link
Contributor Author

@jonalvarezz

😄 i think it is – the value of the non-existent field is falsy, the condition doesn't meet

💀

@grumpygreenguy grumpygreenguy disabled auto-merge July 1, 2024 19:06
@@ -14,6 +14,7 @@
"ts-node": {
"esm": true,
"experimentalResolver": true,
"experimentalSpecifierResolution": "node"
"experimentalSpecifierResolution": "node",
"transpileOnly": true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this to mitigate the error:NULL messages from mocha; see this issue /cc @jonalvarezz @0xverin

Copy link
Contributor

@jonalvarezz jonalvarezz left a comment

Choose a reason for hiding this comment

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

tested locally via https://github.com/litentry/client-sdk/pull/35's update.

looking good. thanks, Ariel!

@grumpygreenguy grumpygreenguy merged commit 9b7946b into dev Jul 2, 2024
33 checks passed
@grumpygreenguy grumpygreenguy deleted the p-609-refactoring-struct-requestvcresultorerror branch July 2, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C0-breaking Breaking change that will cause existing client to break
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants