CCIP-4474 manual exec lbtc #8
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Coverage Report |
andrevmatos
requested changes
Dec 9, 2024
src/lib/offchain.ts
Outdated
Comment on lines
168
to
182
| const attestations: (string | undefined)[] = [] | ||
| for (const [i, _] of msg.tokenAmounts.entries()) { | ||
| const destTokenData = parseSourceTokenData(msg.sourceTokenData[i]).extraData | ||
| // If attestation is required, SourceTokenData.extraData will be 32 bytes ('0x' + 64 hex chars) | ||
| // otherwise attestation is not required | ||
| if (destTokenData.length !== 66) { | ||
| attestations.push(undefined) | ||
| continue | ||
| } | ||
| // check that this is LBTC indeed | ||
| if (lbtcDepositHashes.includes(destTokenData)) { | ||
| attestations.push(await getLbtcAttestation(destTokenData, isTestnet)) | ||
| } | ||
| } | ||
| return attestations |
There was a problem hiding this comment.
This function return usage requires exactly 1 entry per tokenAmount, otherwise the align by index inside fetchOffchainTokenData is not gonna work properly; But your loop here doesn't guarantee it'll return exactly tokenAmounts.length entries (e.g. when destTokenData.length == 66 but lbtcDepositHashes doesn't include it).
I'd suggest a Promise.all(tokenAmounts.map) (which guarantees/expresses better 1-to-1 result), and inside, you do your checks/requests, return the result if you have it, and let it fall through to return undefined in the end if some condition isn't met.
src/lib/offchain.ts
Outdated
|
|
||
| type AttestationResponse = | ||
| | { error: 'string' } | ||
| | { status: 'pending_confirmations' } | ||
| | { status: 'complete'; attestation: string } | ||
|
|
||
| type LombardAttestation = { status: string; message_hash: string; attestation: string } |
There was a problem hiding this comment.
Nit: I see you compare status here with some constant. You don't need to extract that const in a global var, but if you can get a small list of possible status, it'd be nice to hardcode them as a union of string literals here, as it'll aid the constant comparison down there.
Suggested change
| type LombardAttestation = { status: string; message_hash: string; attestation: string } | |
| type LombardAttestation = { status: string; message_hash: string; attestation: string } |
andrevmatos
previously approved these changes
Dec 9, 2024
|
Can you add a CHANGELOG entry, please? |
andrevmatos
approved these changes
Dec 10, 2024
andrevmatos
pushed a commit
that referenced
this pull request
Dec 10, 2024
Add lbtc manual exec feature. If destTokenData is more than 32 bytes - attestation is disabled onchain else - call lombard attestation api for a proof and include it as offchain token data
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add lbtc manual exec feature.
If destTokenData is more than 32 bytes - attestation is disabled onchain
else - call lombard attestation api for a proof and include it as offchain token data