Expose message retrieval by ID or TxHash (API) #97
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ccip-sdk/src/api/index.ts
Outdated
| return raw.map((ta) => ({ | ||
| token: ta.tokenAddress, | ||
| amount: BigInt(ta.amount), | ||
| // TODO: API does not provide pool addresses - these require on-chain lookup |
There was a problem hiding this comment.
Let's discuss what's the best approach here.
ccip-sdk/src/api/index.ts
Outdated
| ) | ||
| } | ||
|
|
||
| const url = `${this.baseUrl}/v1/messages/${encodeURIComponent(messageId)}` |
There was a problem hiding this comment.
Let's keep in mind the order of dependencies between the different components and their rollout plans before merging this.
There was a problem hiding this comment.
Retargeted the PR base to feat/APIv2 (A staging branch for the purpose of behaviour that only works with the new schema)
EDIT: Retargeted to main again after our conversation in the sync. It's fine to merge behaviour that targets the V2 API to main.
Coverage Report |
PabloMansanet
force-pushed
the
retrieve_from_api_by_ID
branch
from
e56402a to
4ccdb3e
Compare
|
Also, we need to update the changelog and cli/sdk under /docs folder |
| }) | ||
|
|
||
| it('should throw CCIPApiClientNotAvailableError when --no-api flag is set', async () => { | ||
| it('should throw CCIPApiClientNotAvailableError when --noapi flag is set', async () => { |
There was a problem hiding this comment.
FYI @aelmanaa I renamed --no-api to --noapi because the arg parser takes the former as a negation of the api flag, which doesn't exist, and that breaks the command.
Not sure if this came up in testing before, but --no-api won't work unless we add --api.
There was a problem hiding this comment.
Okay @PabloMansanet - just please make sure all the mentions to --no-api are replaced then. I can see in line 368 of /docs/cli/index.md
**Note:** This command requires CCIP API access. It will fail if `--no-api` flag is used.
| `Failed to retrieve message ${messageId} via API and RPC.\n API: ${apiMsg}\n RPC: ${rpcMsg}`, | ||
| { | ||
| ...options, | ||
| isTransient: apiError?.isTransient ?? false, |
There was a problem hiding this comment.
| isTransient: apiError?.isTransient ?? false, | |
| isTransient: apiError?.isTransient || rpcError?.isTransient || false |
if API is disabled , the error is an rpcone (we should check isTransient )
|
|
||
| this.logger.debug(`CCIPAPIClient: GET ${url}`) | ||
|
|
||
| const response = await this._fetch(url) |
There was a problem hiding this comment.
The API client methods (getMessageById, getMessageIdsInTx, getLaneLatency) currently have no timeout protection - a slow or unresponsive API could cause indefinite hangs. Consider adding a fetchWithTimeout helper using AbortController - example:
private async fetchWithTimeout(url: string, timeoutMs = 30000): Promise
The timeout could be configurable via CCIPAPIClientContext for users with specific latency requirements.
what do you think?
| }) | ||
|
|
||
| it('should throw CCIPApiClientNotAvailableError when --no-api flag is set', async () => { | ||
| it('should throw CCIPApiClientNotAvailableError when --noapi flag is set', async () => { |
There was a problem hiding this comment.
Okay @PabloMansanet - just please make sure all the mentions to --no-api are replaced then. I can see in line 368 of /docs/cli/index.md
**Note:** This command requires CCIP API access. It will fail if `--no-api` flag is used.
| let getChain: ChainGetter | undefined | ||
| const ensureChains = async () => { | ||
| if (!getChain) { | ||
| getChain = fetchChainsFromRpcs(ctx, argv) |
There was a problem hiding this comment.
NIT (okay for now because I don't see any parallelism): ensureChains memoizes the result but not the Promise (while fetchChainsFromRpcs seems to be a pretty slow operation). Currently safe since calls are sequential, but consider memoizing the Promise for future-proofing if parallel calls are ever added.
| CCIPTransactionNotFoundError, | ||
| CCIPMessageIdNotFoundError, | ||
| CCIPMessageIdValidationError, | ||
| CCIPMessageRetrievalError, |
There was a problem hiding this comment.
The error handling section documents errors for getMessageById, but getMessageIdsInTx throws additional errors that aren't documented: CCIPMessageNotFoundInTxError, CCIPUnexpectedPaginationError, CCIPArgumentInvalidError. Could you consider adding these for consistency?
4 participants
Unit tested and tested manually through a localhost deployment of the latest CCIP API:
What this is
This PR introduces a method to the CCIP API Client that requests a message by messageId, agnostic about any other environment data (CCIP message IDs are unique so this will work across chains and onramps).
To verify the behaviour, I also added a CLI command in the same style as the one for
getLatency. The fields exposed by it are a smaller subset but can be expanded to be more informative if needed.Return type concerns
Currently, the return type of the method is a composite of a
Partialform ofCCIPRequest, and aMetadatatype. The reasons for this are:CCIPRequestis available from the API alone.CCIPRequest.This works fine for now (and could be merged as-is if needed) but ideally we iterate on this until we find a clean common type that we can reuse directly, with the tradeoff of having some optional fields. Reaching this will likely require tweaks on both sides. Here are the rough edges that I found, and how I think we should tackle them:
MessageStatusenum (SDK side) does not yet support 1.7 statuses, where the one in the API does. This is fine and shouldn't be a blocker, because the enums are completely compatible otherwise. Before the SDK supports 1.7, an attempt to retrieve a 1.7 message will fail on validation, which is the expected behaviour.TokenAmountsin the API currently only include the token address on source + the transferred amount, where the SDK expects detailed info including source pool address, destination pool address, extraData and destGasAmount. We could expose this in the API, but I wanted to check with @andrevmatos whether there's a usecase for these fields or whether we can KISS for now. EDIT: Opened a jira ticket to handle the API change needed to expose this data.APICCIPRequestMetadata) which are retrieved from the API and don't exist directly in theCCIPRequesttype. I have no strong opinion on whether these should be integrated intoCCIPRequestor should remain on the side.CCIPRequesttype fields and the API responses. For example, thenonceoronrampmay not exist in the API response whereCCIPRequestexpects it. In case of absence, I've defaulted them to empty values for this draft, but it would be good to make informed decisions on why they're optional or not in either side, unify the ones that can be unified, and converge on what to do otherwise.