offchain - resurrect multi usdc transfers per msg by dimkouv · Pull Request #633 · smartcontractkit/ccip

dimkouv · 2024-03-22T10:49:28Z

Knowledge-sharing for reviewers

For each token of a msg we find the token pool and call lock_or_burn.
For USDC pool, the usdc contract we call emits a UsdcMessageSent event.
This events are picked up by the log poller.

The problem

For a given ccip msg (tx hash, log index) we called:

usdcreader.GetLastUSDCMessagePriorToLogIndexInTx(logIndex, txHash)

But this only gives us the Last usdc message log.
A ccip message can contain multiple usdc token transfers with one log emitted for each.

The fix

I am updating the method above to also accept usdcTokenIndexOffset

usdcreader.GetUSDCMessagePriorToLogIndexInTx(logIndex, usdcTokenIndexOffset, txHash) ([]byte, error)

Here is an example:

msg.tokenAmounts = [
   {token: "0xusdc", amount: 12} // offset=2
   {token: "0xSomehingElse", amount: 3123}
   {token: "0xusdc", amount: 14} // offset=1
   {token: "0xusdc", amount: 114} // offset=0 (that's what we would return without this fix)
]

The reason that we essentially ignore non-usdc tokens is that for a message containing multiple usdc transfers the logs returned by the logPoller do not contain non-usdc logs, so we need to have that offset to pick the correct log.

…ge (#632) ## Motivation ## Solution

github-actions · 2024-03-22T10:49:47Z

I see you updated files related to core. Please run pnpm changeset to add a changeset.

RensR

I feel like the test coverage around actually using multiple usdc tokens in a single msg is missing

AnieeG · 2024-03-28T06:47:47Z

I feel like the test coverage around actually using multiple usdc tokens in a single msg is missing

Just updated the test to include this scenario. one msg 2 usdc tokens, one non-usdc token

mateusz-sekara · 2024-03-28T11:32:33Z

core/services/ocr2/plugins/ccip/internal/ccipdata/usdc_reader.go

+	usdcTokenIndex := (len(allUsdcTokensData) - 1) - usdcTokenIndexOffset
+
+	if usdcTokenIndex < 0 || usdcTokenIndex >= len(allUsdcTokensData) {
+		return nil, errors.Errorf("no USDC message found prior to log index %d, usdc token:%d in tx %s, usdcTokenIndex=%d",


IMO it's better to log errors with so many details because it's easier to search in Loki then. The returned error could be simplified then

mateusz-sekara · 2024-03-28T11:35:25Z

core/services/ocr2/plugins/ccip/internal/ccipdata/usdc_reader.go

-		current := logs[len(logs)-i-1]
+	// collect the logs with log index less than the provided log index
+	allUsdcTokensData := make([][]byte, 0)
+	for _, current := range logs {


I think we miss tests covering this logic, no?

mateusz-sekara · 2024-03-28T11:37:59Z

core/services/ocr2/plugins/ccip/internal/ccipdata/usdc_reader.go

 }

-func (u *USDCReaderImpl) GetLastUSDCMessagePriorToLogIndexInTx(ctx context.Context, logIndex int64, txHash string) ([]byte, error) {
+func (u *USDCReaderImpl) GetUSDCMessagePriorToLogIndexInTx(ctx context.Context, logIndex int64, usdcTokenIndexOffset int, txHash string) ([]byte, error) {


Correct me if I'm wrong, but the only difference is that we fetch all the USDC logs from a single TX and the we pick the right one based on the usdcTokenIndexOffset, no?

IIUC, having a CCIP message with multiple USDC transfers will always fetch all the logs from a single TX (u.lp.IndexedLogsByTxHash), pick the correct one, and then discard the rest of them. Also, it would be done multiple times, right?

Very nice observation, we can improve it as a follow up, by passing a list of tokens probably.
I would prefer not to include it here as it requires changes in several places not related to this change.

Sounds good, please make sure that ticket for that followup is created ;)

mateusz-sekara · 2024-03-28T11:41:26Z

core/services/ocr2/plugins/ccip/internal/ccipdata/usdc_reader.go

+	// if usdcTokenIndexOffset is 0 we select usdc2
+	// if usdcTokenIndexOffset is 1 we select usdc1
+	// The message logs are found using the provided transaction hash.
+	GetUSDCMessagePriorToLogIndexInTx(ctx context.Context, logIndex int64, usdcTokenIndexOffset int, txHash string) ([]byte, error)


PriorToLogIndex? IMO it's more like GetUSDCMessageBasedOnTheLogIndex

I don't like both names :D Let's rename it in the follow-up with the performance improvement.

GetUSDCMessageByLogIndex? This is the convention we have in LogPoller (but not in all places :P). PriorToIndex is very misleading to me, because it sounds like "give me all logs from [0, tokenIndex]"

Message singular and PriorToIndex implies the last singular log before the given index 🤷🏼

mateusz-sekara · 2024-03-28T11:42:51Z

core/services/ocr2/plugins/ccip/tokendata/usdc/usdc.go

+		return nil, fmt.Errorf("invalid token index %d for msg with %d tokens", tokenIndex, len(msg.TokenAmounts))
+	}
+
+	offsetFromLastUsdcToken := 0


I would extract that to a separate method that just returns a proper index of usdc token

mateusz-sekara · 2024-03-28T11:53:44Z

core/services/ocr2/plugins/ccip/internal/ccipdata/usdc_reader.go

-	// GetLastUSDCMessagePriorToLogIndexInTx returns the last USDC message that was sent
-	// before the provided log index in the given transaction.
-	GetLastUSDCMessagePriorToLogIndexInTx(ctx context.Context, logIndex int64, txHash string) ([]byte, error)
+	// GetUSDCMessagePriorToLogIndexInTx returns the specified USDC message data.


Correct me if I'm wrong, but it works as follows

for i := range msg.tokens { if usdc { log := fetchLogFromDatabase(i) attestation := fetchAttestation(log, i) } }

This means that 100 usdc messages will equal 100 database calls, right?
Whereas we could be more efficient here probably by doing sth like this. This assumes that all logs are in the same TX, I'm not sure if this is a valid assumption

// grouping by type first usdcMsgs := []msgs for i := range msg.tokens { if usdc { usdcMsgs = append(usdcMsgs, msg.tokens[i]) } } logs := fetchLogFromDatabase() for i:= range usdcMessages { fetchLogFromDatabase(logs[i]. i) }

sounds like a nice improvement, we can do as a follow up since it requires more interface changes that I don't want to include in this pr

While nice in theory, the odds of multiple usdc txs in 1 tx is so low it's totally fine to punt imo

RensR · 2024-03-28T16:55:46Z

core/services/ocr2/plugins/ccip/tokendata/usdc/usdc.go

+func (s *TokenDataReader) getUSDCMessageBody(
+	ctx context.Context,
+	msg cciptypes.EVM2EVMOnRampCCIPSendRequestedWithMeta,
+	tokenIndex int,


Why isnt this a uint, it should never be <0 correct?

A bit less casting for comparisons e.g. with len. Imo not really important

jasonmci

Approved

## Knowledge-sharing for reviewers 1. For each token of a msg we find the token pool and call lock_or_burn. 2. For USDC pool, the usdc contract we call emits a `UsdcMessageSent` event. 3. This events are picked up by the log poller. ## The problem For a given ccip msg (tx hash, log index) we called: ``` usdcreader.GetLastUSDCMessagePriorToLogIndexInTx(logIndex, txHash) ``` But this only gives us the `Last` usdc message log. A ccip message can contain multiple usdc token transfers with one log emitted for each. ## The fix I am updating the method above to also accept `usdcTokenIndexOffset` ``` usdcreader.GetUSDCMessagePriorToLogIndexInTx(logIndex, usdcTokenIndexOffset, txHash) ([]byte, error) ``` Here is an example: ``` msg.tokenAmounts = [ {token: "0xusdc", amount: 12} // offset=2 {token: "0xSomehingElse", amount: 3123} {token: "0xusdc", amount: 14} // offset=1 {token: "0xusdc", amount: 114} // offset=0 (that's what we would return without this fix) ] ``` The reason that we essentially ignore non-usdc tokens is that for a message containing multiple usdc transfers the logs returned by the logPoller do not contain non-usdc logs, so we need to have that offset to pick the correct log. --------- Co-authored-by: AnieeG <anindita.ghosh@smartcontract.com> Co-authored-by: Anindita Ghosh <88458927+AnieeG@users.noreply.github.com>

dimkouv and others added 3 commits March 21, 2024 15:08

plumbing

41597b0

enabling 2 USDC tokens in test

108935b

Ccip-1896 - usdc test to support usdc and non-usdc token in one messa…

d6b8aca

…ge (#632) ## Motivation ## Solution

dimkouv temporarily deployed to sdlc March 22, 2024 10:49 — with GitHub Actions Inactive

dimkouv changed the title ~~offchain - ressurrect multi usdc transfers per msg~~ offchain - resurrect multi usdc transfers per msg Mar 22, 2024

Merge branch 'ccip-develop' into dk/ressurrect-multiusdc-transfers

48bea56

dimkouv temporarily deployed to sdlc March 22, 2024 10:50 — with GitHub Actions Inactive

resolve conflicts

533f9b6

dimkouv temporarily deployed to sdlc March 26, 2024 12:34 — with GitHub Actions Inactive

implement the fix

455a7f7

dimkouv temporarily deployed to sdlc March 26, 2024 13:25 — with GitHub Actions Inactive

lint fix

a7e8732

dimkouv temporarily deployed to sdlc March 26, 2024 13:39 — with GitHub Actions Inactive

dimkouv and others added 2 commits March 27, 2024 14:32

fix

d231887

merge with ccip develop and fix tests

0115dd4

RensR reviewed Mar 28, 2024

View reviewed changes

mateusz-sekara reviewed Mar 28, 2024

View reviewed changes

dimkouv added 2 commits March 28, 2024 15:02

fixes

f439ce2

Merge branch 'ccip-develop' into dk/ressurrect-multiusdc-transfers

e8bd726

mateusz-sekara approved these changes Mar 28, 2024

View reviewed changes

RensR reviewed Mar 28, 2024

View reviewed changes

RensR approved these changes Mar 28, 2024

View reviewed changes

jasonmci approved these changes Mar 28, 2024

View reviewed changes

dimkouv closed this Mar 28, 2024

Conversation

dimkouv commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Knowledge-sharing for reviewers

The problem

The fix

Uh oh!

github-actions bot commented Mar 22, 2024

Uh oh!

RensR left a comment

Choose a reason for hiding this comment

Uh oh!

AnieeG commented Mar 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimkouv Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RensR Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmci left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dimkouv commented Mar 22, 2024 •

edited

Loading

dimkouv Mar 28, 2024 •

edited

Loading

RensR Mar 28, 2024 •

edited

Loading