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

Improvements #195

Merged
merged 11 commits into from
May 10, 2024
Merged

Improvements #195

merged 11 commits into from
May 10, 2024

Conversation

mohsinriaz17
Copy link
Member

Description

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

throw new error removed the error object which contain actual error details from server
made prepareAndStoreKeyShares into smaller function which uses prepareStoreablePayloadForShares
this can be used to prepare shares to be stored with signing

added identifyAndStoreFailedShares which takes data from prepareAndStoreKeyShares and retry with only failed items

naming convention improvememnts
Copy link

vercel bot commented May 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ternoa-js ❌ Failed (Inspect) May 10, 2024 10:46am

@@ -87,6 +87,9 @@ export type TeeSharesStoreType = {
enclaveAddress: string
operatorAddress: string
enclaveSlot: number
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure anyone uses this type outside of the SDK itself, but those new additions must be marked as breaking changes in case.

@@ -55,8 +55,8 @@ export const retryPost = async <T>(fn: () => Promise<any>, n: number): Promise<T
} catch (err: any) {
lastError = {
isRetryError: true,
status: "SDK_RETRY_POST_ERROR",
message: err?.message ? err.message : JSON.stringify(err),
status: err.status,
Copy link
Member

Choose a reason for hiding this comment

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

Updating the status with a proper status 500 from HTTP is a breaking change. If the function is successful, it will return the enclave status which is a string: STORESUCCESS, DATABASEFAILURE, KEYNOTEXIST (...) while it will be a Number in case of failure. The developer will have to filter the type if handling the response is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed it so internal status is now a string on fail

if (kind !== "secret" && kind !== "capsule") {
throw new Error(`${Errors.TEE_ERROR}: Kind must be either "secret" or "capsule"`)
}
const teeEnclaves = await getTeeEnclavesBaseUrl(clusterId)
let isShareAvailable = false
let i = 0
while (isShareAvailable !== true && i <= teeEnclaves.length - 1) {
console.log("checking enclave i", i, "---", teeEnclaves[i])
Copy link
Member

Choose a reason for hiding this comment

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

remove the log

}
}),
)

shares = shares.filter((x) => x !== undefined)
if (shares.length < SSSA_THRESHOLD) {
throw new Error(`${Errors.TEE_RETRIEVE_ERROR} - Shares could not be retrieved: ${errors[0]}`)
throw { error: `${Errors.TEE_RETRIEVE_ERROR} - Shares could not be retrieved`, details: errors }
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure anyone uses this fn outside of Ternoa's internal use, but this exception object replacing the string error, might need to be marked as breaking changes in case.

Copy link
Member Author

Choose a reason for hiding this comment

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

reverted to be string as before

}),
)

return teeRes.map((enclaveRes, i) => {
const payload = payloads[i]
return teeRes.map((enclaveRes) => {
Copy link
Member

Choose a reason for hiding this comment

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

the returned structure looks good and not breaking change. Just more additions

throw new Error(err)
if (err.response) throw { status: err.response.status, data: err.response.data }
throw { status: 500, data: "ECONNREFUSED", error: err }
// throw new Error('url:' + url + ' Error:' + err)
Copy link
Member

Choose a reason for hiding this comment

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

this new answer structure should be considered and marked as a breaking change from a developer's pov.

Same for the two remaining ones.

@mohsinriaz17 mohsinriaz17 merged commit c2a40dd into dev May 10, 2024
2 of 4 checks passed
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.

2 participants