Skip to content

Conversation

@huangzhen1997
Copy link
Contributor

@huangzhen1997 huangzhen1997 commented Oct 31, 2025

@huangzhen1997 huangzhen1997 marked this pull request as ready for review October 31, 2025 01:25
@huangzhen1997 huangzhen1997 requested a review from a team as a code owner October 31, 2025 01:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the GetFeeQuoterTokenUpdates method in the TON accessor to fix how token price data is fetched. Instead of calling a batch tokenPrices method, it now fetches token prices individually using the tokenPrice getter method with the new FetchResult helper.

Key changes:

  • Replaced batch token price fetching with individual per-token calls using FetchResult
  • Added FetchResult helper method to TimestampedPrice struct with a new tokenPriceGetter constant
  • Added integration test coverage for the token price fetching functionality

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
pkg/ccip/chainaccessor/ton_accessor.go Refactored GetFeeQuoterTokenUpdates to fetch prices individually instead of in batch
pkg/ccip/bindings/feequoter/fee_quoter.go Added FetchResult helper method and tokenPriceGetter constant to support individual token price fetching
integration-tests/deployment/cs_test.go Added test case for fetching token prices and fixed import ordering

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +744 to +745
var tokenPrice feequoter.TimestampedPrice
err = tokenPrice.FetchResult(ctx, a.client, block, addr, []interface{}{cell.BeginCell().MustStoreAddr(addrParsed).EndCell().BeginParse()})
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

This implementation makes a separate RPC call for each token in the loop, which could cause significant performance degradation when fetching prices for multiple tokens. The original batch implementation was more efficient. Consider if this sequential approach is necessary or if batch fetching can be preserved.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was my first reaction too from the slack discussion 🤔 Wouldn't this be a fair amount slower or cause additional strain on the endpoint? This GetFeeQuoterTokenUpdates() function can be queried pretty often

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC I had discuss this with @archseer at some point, that it's better to fetch individual config/price so that TVM will not allocate a huge stack space to store them as this map could become very big

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For speed, yeah I'm going to add a errGroup to parallelize the loop

Copy link
Contributor Author

@huangzhen1997 huangzhen1997 Oct 31, 2025

Choose a reason for hiding this comment

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

This was the discussion. But it was slightly different as DestChainConfig is much bigger than timestampPrice. We are less likely hit the 255 stack limit for fetching token prices, so maybe it's better to do batching if GetFeeQuoterTokenUpdates() gets called very often.

Copy link
Contributor Author

@huangzhen1997 huangzhen1997 Oct 31, 2025

Choose a reason for hiding this comment

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

Discussed with @ogtownsend offline, only LINK and the native token are used most of the time as input tokens for GetFeeQuoterTokenUpdates, so batching is not implemented for now to keep everything simple and clean. Added a comment to mention that batching can be added later if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are planing to merge this as-is and leave the parallelization for later, should we add a TODO here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right now parallelization is not needed for just two tokens. if we going to support batching it would just be one rpc call

ogtownsend
ogtownsend previously approved these changes Oct 31, 2025
Comment on lines -760 to -766
case nil:
// return zero value
price = ccipocr3.TimestampedUnixBig{
Value: big.NewInt(0),
Timestamp: 0,
}
case cell.Cell:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we not need to explicitly handle this case anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I'm adding that but I notice the contact mustGet is not retuning the right error code but returned stack underflow, checking what's going on

get fun tokenPrice(token: address): TimestampedPrice {
val st = lazy Storage.load();
return st.usdPerToken.mustGet(token, Error.TokenNotSupported as int);
val entry = st.usdPerToken.get(token);
Copy link
Contributor Author

@huangzhen1997 huangzhen1997 Oct 31, 2025

Choose a reason for hiding this comment

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

} else {
result, err = client.RunGetMethod(ctx, block, contractAddr, method, opts...)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return raw error so downstream can parse the error code

result, err := a.client.RunGetMethod(ctx, block, addr, "destinationChainGasPrice", uint64(selector))
// The plugin is built with EVM behaviour in mind: if a value doesn't exist the zero value is returned
if execError, ok := err.(ton.ContractExecError); ok && execError.Code == common.ErrUnknownDestChainSelector { //nolint:errorlint // we're guaranteed to get unwrapped error here
if execError, ok := err.(ton.ContractExecError); ok && execError.Code == 24814 { //nolint:errorlint // we're guaranteed to get unwrapped error here
Copy link
Contributor Author

@huangzhen1997 huangzhen1997 Oct 31, 2025

Choose a reason for hiding this comment

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

added todo. The error hard-coded here in order to match on-chain definition. There's a discrepancy of the error code definition

@patricios-space patricios-space merged commit 42e2549 into main Nov 4, 2025
35 checks passed
@patricios-space patricios-space deleted the fix-ton-accessor branch November 4, 2025 13:24
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.

3 participants