Revert "Revert "fix: P-441 remove NoEligibleIdentity (#2739)" (#2777)" #2788
Conversation
higherordertech
requested review from
silva-fj,
jonalvarezz and
BillyWooo
and removed request for
silva-fj
tee-worker/client-api/parachain-api/prepare-build/interfaces/sidechain/definitions.ts
Outdated
Show resolved
Hide resolved
primitives/core/src/error.rs
Outdated
| // error when trying to build vc but no eligible identity is found | ||
| #[codec(index = 11)] | ||
| NoEligibleIdentity, |
There was a problem hiding this comment.
is it safe to remove? or do we need to keep this index unused? I'm thinking of upcoming errors we want to append
There was a problem hiding this comment.
In fact, if we really want to retain the maximal compatibility, we could leave this enum unchanged (so does definitions.ts)
So this will just become an error type that will never be emitted.
Good thing is you don't have to change anything in SDK. @jonalvarezz @higherordertech
There was a problem hiding this comment.
I'm fine with either way - they both have arguments
Will leave it to you to decide
higherordertech
force-pushed
the
P-441-remove-NoEligibleIdentity
branch
from
925740b to
2a38393
Compare
higherordertech
force-pushed
the
P-441-remove-NoEligibleIdentity
branch
from
2a38393 to
3e3ea00
Compare
| @@ -66,7 +66,7 @@ pub enum ErrorDetail { | |||
| // of verification data | |||
| #[codec(index = 10)] | |||
| VerifyWeb3SignatureFailed, | |||
| // error when trying to build vc but no eligible identity is found | |||
| // this is deprecated as this should not be emitted anymore to not leak the information, kept here so client side type definition do not need change | |||
| #[codec(index = 11)] | |||
| NoEligibleIdentity, | |||
There was a problem hiding this comment.
This never-used error type is retained now, but with deprecated note
higherordertech
force-pushed
the
P-441-remove-NoEligibleIdentity
branch
from
3e3ea00 to
6617764
Compare
There was a problem hiding this comment.
But I'm wondering if this PR is still up-to-date, because we don't send any "RequestVcFailed" events to parachian now...
So there's no information leak even for NoEligibleIdentity.
On the other side, it does help reduce the response time and alleviate the server load (so that it returns early when possible)
primitives/core/src/error.rs
Outdated
| @@ -90,7 +91,6 @@ impl fmt::Debug for ErrorDetail { | |||
| ErrorDetail::WrongWeb2Handle => write!(f, "WrongWeb2Handle"), | |||
| ErrorDetail::UnexpectedMessage => write!(f, "UnexpectedMessage"), | |||
| ErrorDetail::VerifyWeb3SignatureFailed => write!(f, "VerifyWeb3SignatureFailed"), | |||
| ErrorDetail::NoEligibleIdentity => write!(f, "NoEligibleIdentity"), | |||
There was a problem hiding this comment.
Then you'll need to keep this
| @@ -86,7 +86,7 @@ export default { | |||
| UnexpectedMessage: "Null", | |||
| __Unused_WrongSignatureType: "Null", | |||
| VerifyWeb3SignatureFailed: "Null", | |||
| NoEligibleIdentity: "Null", | |||
| __Unused_NoEligibleIdentity: "Null", | |||
There was a problem hiding this comment.
Now that we keep NoEligibleIdentity, I suggest we leave it unchagned.
It's "unused" only from logic/code perspective, but the type is actually there.
There was a problem hiding this comment.
Sounds a plan, so the change becomes:
1 No logic will emit that, including StfError::RequestVCFailed, so to not leak it
2 NoEligibleIdentity and it's type definition still remains unchanged
…ypescript definition will need change, however NoEligibleIdentity will not be used anymore
higherordertech
force-pushed
the
P-441-remove-NoEligibleIdentity
branch
from
2b3fac4 to
5dff202
Compare
|
Hi guys @higherordertech @Kailai-Wang , a bit confused. What's the conclusion?
|
|
Don't need it anymore. Simply close it. |
This reverts commit dbcac65.
Context
Labels
Please apply following PR-related labels when appropriate:
C0-breaking: if your change could break the existing client, e.g. API change, critical logic changeC1-noteworthy: if your change is non-breaking, but is still worth noticing for the client, e.g. reference code improvementHow (Optional)
Testing Evidences
Please attach any relevant evidences if applicable