Offchain usdc #112

RensR · 2023-09-05T11:43:50Z

Motivation

USDC is supported onchain, now we need to add offchain support

Solution

Add attestation logic and a new USDC service to manage USDC related logic

PENDING main repo logpoller and index changes

core/chains/evm/logpoller/orm.go

dimkouv · 2023-09-07T13:39:52Z

core/chains/evm/logpoller/orm.go

+			AND tx_hash = $3
+			AND event_sig = $4
+			ORDER BY (evm_logs.block_number, evm_logs.log_index)`,
+		utils.NewBig(o.chainID), txHash.Bytes(), eventSig.Bytes())


Note: Make sure query is properly hitting indexes.

Yep, please make sure it doesn't hurt db performance, I described how to create a simple test on a local machine and test it

Probably we miss the index for tx_hash. Postgres will be able to performantly pick up data using index only for chain_id and event_sig. Then will have to perform filtering on the remaining part using tx_hash

Example on my local machine

Sort (cost=575.91..575.92 rows=1 width=297) (actual time=213.647..259.292 rows=100000 loops=1) Sort Key: (ROW(block_number, log_index)) Sort Method: external merge Disk: 31240kB -> Index Scan using idx_evm_logs_ordered_by_block_and_created_at on logs (cost=0.55..575.90 rows=1 width=297) (actual time=0.070..89.148 rows=100000 loops=1) Index Cond: ((evm_chain_id = '1'::numeric) AND (event_sig = '\xa32c5f1d88735034143171f18fbb4d447fbbe0fbf4c98733d54092a081ba5d2a'::bytea)) Filter: (tx_hash = '\xbb0f07700587fee22c5863ed75a5e05b68fcac4763449a582987fa4e8e9a2d0f'::bytea)

dimkouv · 2023-09-07T13:48:11Z

core/services/ocr2/plugins/ccip/ccipevents/client.go

@@ -39,6 +41,9 @@ type Client interface {
 	// GetExecutionStateChangesBetweenSeqNums returns all the execution state change events for the provided message sequence numbers (inclusive).
 	GetExecutionStateChangesBetweenSeqNums(ctx context.Context, offRamp common.Address, seqNumMin, seqNumMax uint64, confs int) ([]Event[evm_2_evm_offramp.EVM2EVMOffRampExecutionStateChanged], error)

+	// 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 common.Hash) ([]byte, error)


It should be cleaner to change the method signature to:

GetLastUSDCMessagePriotToSeqNum(ctx, seqNum)

That way you will have to make the two calls inside the ccipevents/ method, so the caller code is cleaner, it only needs to know the sequence number to access that method.

It would need the params from GetSendRequestsBetweenSeqNums as well, so the onRampAddress. Might be worth it though

resolved in other ways, please check if the new way works for you

Maybe update this method to accept a ccip msg (trying to keep the interface close to ccip not to evm, that way it's going to be easier to add non-evm).

GetLastUSDCMessagePriorToCcipMsg(ctx context.Context, msg)

logIndex won't work for e.g. solana

dimkouv · 2023-09-07T13:53:07Z

core/services/ocr2/plugins/ccip/execution_plugin.go

+
+	tokenDataProviders := make(map[common.Address]offchaintokendata.Provider)
+
+	// TODO the called function only uses the chainId to get the message transmitter address


The filter register/unregister logic becomes weird. But I'm not sure if this can be improved :S

Jup, happy to hear any suggestions

if we put the relevant addresses in the plugin config this problem goes away right? Maybe just put a USDCConfig struct in the plugin config like:

type USDCConfig struct { AttestationAPI string `json:"attestationAPI"` MessageTransmitterAddress common `json:"MessageTransmitterAddress"` TokenAddress common `json:"MessageTransmitterAddress"` } func (uc USDCConfig) Validate(chainID) error { // run basic sanity on the addresses}

Do you mean the job spec?

dimkouv · 2023-09-07T13:58:48Z

core/services/ocr2/plugins/ccip/execution_reporting_plugin.go

@@ -66,6 +67,7 @@ type ExecutionPluginConfig struct {
 	sourceClient             evmclient.Client
 	destGasEstimator         gas.EvmFeeEstimator
 	leafHasher               hasher.LeafHasherInterface[[32]byte]
+	tokenDataProviders       map[common.Address]offchaintokendata.Provider


I am thinking about replacing the map with:

type TokenDataProvider interface { IsAttestationComplete(ctx context.Context, token common.Address, seqNum uint64) (ready bool, attestation []byte, err error) GetSourceLogPollerFilters(token common.Address) []logpoller.Filter GetSourceToken(token common.Address) common.Address }

i.e. accept token address on each method call as first arg, I think that it would make the plugin code cleaner

After implementing TokenDataProvider for another token you also won't have to touch plugin code.

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

dimkouv · 2023-09-07T14:22:46Z

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

+		return AttestationResponse{}, err
+	}
+	req.Header.Add("accept", "application/json")
+	res, err := http.DefaultClient.Do(req)


I would suggest to hook a properly configured http client instance to the struct instead of using the http.DefaultClient.

Isn't there any lib in our codebase to deal with http calls? which leads to not do manual stuff like req.Header.Add("accept", "application/json") -> read body -> parse body -> etc...

Looks like this is the best method. DefaultClient already is a reused object so there doesn't appear to be much overhead.

core/chains/evm/logpoller/observability.go

mateusz-sekara · 2023-09-11T12:07:13Z

core/chains/evm/logpoller/orm.go

+	var logs []Log
+	err := q.Select(&logs, `
+		SELECT * FROM evm_logs 
+			WHERE evm_logs.evm_chain_id = $1


Prefixing by table name is not needed here (we don't even use joins here). IMO it's more confusing with prefixes

I copied it from the other functions, they all do it

mateusz-sekara · 2023-09-11T12:08:49Z

core/chains/evm/logpoller/orm.go

+			AND tx_hash = $3
+			AND event_sig = $4
+			ORDER BY (evm_logs.block_number, evm_logs.log_index)`,
+		utils.NewBig(o.chainID), txHash.Bytes(), eventSig.Bytes())


Yep, please make sure it doesn't hurt db performance, I described how to create a simple test on a local machine and test it

mateusz-sekara · 2023-09-11T12:17:43Z

core/chains/evm/logpoller/log_poller.go

@@ -1005,6 +1006,10 @@ func (lp *logPoller) LatestBlockByEventSigsAddrsWithConfs(fromBlock int64, event
 	return lp.orm.SelectLatestBlockNumberEventSigsAddrsWithConfs(fromBlock, eventSigs, addresses, confs, qopts...)
 }

+func (lp *logPoller) IndexedLogsByTxHash(eventSig common.Hash, txHash common.Hash, qopts ...pg.QOpt) ([]Log, error) {


Make sure to backport these changes to the main repo

core/services/ocr2/plugins/ccip/offchaintokendata/offchain_data_provider.go

core/services/ocr2/plugins/ccip/execution_reporting_plugin.go

mateusz-sekara · 2023-09-11T12:32:09Z

core/services/ocr2/plugins/ccip/execution_reporting_plugin.go

+		var tokenData [][]byte
+		for _, token := range msg.TokenAmounts {
+			if offchainTokenDataProvider, ok := r.config.tokenDataProviders[token.Token]; ok {
+				ready, attestation, err2 := offchainTokenDataProvider.IsAttestationComplete(context.TODO(), msg.SequenceNumber)


I agree, it probably needs some pooling or running those concurrently in different go routines; also a good idea with timeouts.

wrap error, improved url formatting, orm fix

# Conflicts: # core/services/ocr2/plugins/ccip/execution_plugin.go

# Conflicts: # core/services/ocr2/plugins/ccip/execution_plugin.go # core/services/ocr2/plugins/ccip/execution_reporting_plugin.go # core/services/ocr2/plugins/ccip/execution_reporting_plugin_test.go # core/services/ocr2/plugins/ccip/internal/ccipevents/client_mock.go

observed logpoller rename IsAttestationComplete mutex in usdc mapping

core/services/ocr2/plugins/ccip/execution_plugin.go

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

core/services/ocr2/plugins/ccip/offchaintokendata/offchain_data_provider.go

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

core/services/ocr2/plugins/ccip/offchaintokendata/offchain_data_provider.go

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

connorwstein · 2023-09-19T03:45:54Z

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

+	ccipSendLogIndex := int64(ccipSendRequest.Data.Raw.Index)
+
+	// Get the USDC message body
+	usdcMessageBody, err := s.sourceChainEvents.GetLastUSDCMessagePriorToLogIndexInTx(ctx, ccipSendLogIndex, ccipSendTxHash)


Hmm I actually wonder if this is better suited to getReportsWithSendRequests and just part of meta in sendEventWithMeta for USDC sends? That way we load all relevant log data in one place and this component doesn't have to know about log stuff at all. It just can do simple dead simple memoization based on the calldata->responses. Also would help keep buildBatch more pure/testable

Hmm true but that means spilling the USDC logic into the rest of the codebase. It's either spreading USDC or spreading log lookups. I'd prefer to not spill USDC, as there might be many cases like USDC and log getting is fairly generic. I see the value in keeping buildBatch more pure, but with the proper testing framework it should be extremely easy to mock out USDC logic, meaning it would remain pure from the tests perspective

connorwstein · 2023-09-19T04:01:36Z

core/services/ocr2/plugins/ccip/execution_plugin.go

+
+	tokenDataProviders := make(map[common.Address]offchaintokendata.Provider)
+
+	// TODO the called function only uses the chainId to get the message transmitter address


if we put the relevant addresses in the plugin config this problem goes away right? Maybe just put a USDCConfig struct in the plugin config like:

type USDCConfig struct { AttestationAPI string `json:"attestationAPI"` MessageTransmitterAddress common `json:"MessageTransmitterAddress"` TokenAddress common `json:"MessageTransmitterAddress"` } func (uc USDCConfig) Validate(chainID) error { // run basic sanity on the addresses}

core/services/ocr2/plugins/ccip/offchaintokendata/usdc/usdc_test.go

# Conflicts: # core/services/ocr2/plugins/ccip/execution_plugin.go # core/services/ocr2/plugins/ccip/execution_reporting_plugin_test.go # core/services/ocr2/plugins/ccip/internal/ccipdata/client_mock.go # core/services/ocr2/plugins/ccip/internal/ccipdata/logpoller.go # core/services/ocr2/plugins/ccip/internal/ccipdata/reader.go

core/services/ocr2/plugins/ccip/execution_plugin.go

core/services/ocr2/plugins/ccip/execution_reporting_plugin.go

dimkouv · 2023-09-20T10:57:56Z

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

+		return []byte{}, err
+	}
+
+	r.cache[msg.SequenceNumber] = tokenData


Maybe use sync.RWMutex and do the lock like this?

that way you make the cache readable while making the readTokenData call, which might take some time.

RLock() try to read from cache RUnlock readTokenData() Lock() put in cache Unlock()

considering the load it's minor though.

maybe also add a comment to highlight that this method will also update the cache, not only read

// ... **and then Updates the cache**

also add some unit test

This will never be called concurrently, but yeah that is a better locking method.

core/services/ocr2/plugins/ccip/internal/chains/chains.go

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

# Conflicts: # core/chains/evm/logpoller/disabled.go # core/services/ocr2/plugins/ccip/execution_plugin.go # core/services/ocr2/plugins/ccip/execution_reporting_plugin.go

connorwstein · 2023-09-20T18:55:34Z

core/services/ocr2/plugins/ccip/execution_plugin.go

+func getTokenDataProviders(pluginConfig ccipconfig.ExecutionPluginJobSpecConfig, onRampAddress common.Address, sourceChainEventClient *ccipdata.LogPollerReader) (map[common.Address]tokendata.Reader, error) {
+	tokenDataProviders := make(map[common.Address]tokendata.Reader)
+
+	if pluginConfig.USDCConfig.AttestationAPI != "" {


nit could invert this to avoid nesting i.e. if == "" early return. Maybe also want an info log indicating we're proceeding without USDC config to speed up debugging?

I was imagining this function to do setup for all future token data providers, therefore having an if for each. With just usdc in mind it makes sense flipping

core/services/ocr2/plugins/ccip/execution_reporting_plugin.go

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

RensR changed the base branch from ccip-develop to usdc-token-pool-support September 5, 2023 11:44

RensR added 3 commits September 5, 2023 13:55

basic attestation service

f478a01

add hard coded LP subscription logic

7a883b7

check usdc messges attestation in batch building (mock)

5468713

RensR requested a review from dimkouv September 5, 2023 13:31

Base automatically changed from usdc-token-pool-support to ccip-develop September 6, 2023 03:59

RensR force-pushed the offchain-usdc branch from 06ff14b to 5468713 Compare September 6, 2023 11:36

RensR added 4 commits September 6, 2023 15:46

add logpoller IndexedLogsByTxHash

1a7b5a9

refactor USDC service to be self contained

1577e56

make offchain token data providers generic

2ac5879

add all usdc token and messageTransmitter addresses

8c2250f

dimkouv reviewed Sep 7, 2023

View reviewed changes

mateusz-sekara reviewed Sep 11, 2023

View reviewed changes

RensR added 6 commits September 13, 2023 13:29

minor code improvements

0f2e9eb

wrap error, improved url formatting, orm fix

Merge branch 'ccip-develop' into offchain-usdc

bb5b50e

# Conflicts: # core/services/ocr2/plugins/ccip/execution_plugin.go

Merge branch 'ccip-develop' into offchain-usdc

a523911

fix evm logs reference

256c29f

review comments

68a70d5

observed logpoller rename IsAttestationComplete mutex in usdc mapping

connorwstein reviewed Sep 19, 2023

View reviewed changes

RensR added 7 commits September 19, 2023 12:38

renames and passing in uri

addbdbb

re-use logindex and tx hash from exec plugin

0ef66b3

cached reader + ReadTokenData rename

239b7ca

fix test, rm unused code

8029d4c

pass in internal.EVM2EVMOnRampCCIPSendRequestedWithMeta

6e31124

extract chains logic and fix linting

0e80c67

dimkouv reviewed Sep 20, 2023

View reviewed changes

RensR added 2 commits September 20, 2023 15:53

add cached reader test & reader mock

7be78f6

more tests, real context and extract into method

142a6d1

RensR added 2 commits September 20, 2023 18:45

Merge branch 'ccip-develop' into offchain-usdc

50a59b2

# Conflicts: # core/chains/evm/logpoller/disabled.go # core/services/ocr2/plugins/ccip/execution_plugin.go # core/services/ocr2/plugins/ccip/execution_reporting_plugin.go

read usdc config from jobspec

acfeef5

connorwstein approved these changes Sep 20, 2023

View reviewed changes

improve logging, reduce exported usdc types

d00da8c

RensR marked this pull request as ready for review September 20, 2023 20:29

RensR requested a review from a team as a code owner September 20, 2023 20:29

RensR changed the title ~~[WIP] Offchain usdc~~ Offchain usdc Sep 20, 2023

RensR force-pushed the offchain-usdc branch from ae97b39 to 4a62657 Compare September 21, 2023 13:05

Merge branch 'ccip-develop' into offchain-usdc

e4f6b07

RensR force-pushed the offchain-usdc branch from 4a62657 to e4f6b07 Compare September 21, 2023 13:06

RensR merged commit 324d8f7 into ccip-develop Sep 21, 2023
69 checks passed

RensR deleted the offchain-usdc branch September 21, 2023 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Offchain usdc #112

Offchain usdc #112

RensR commented Sep 5, 2023 •

edited

dimkouv Sep 7, 2023

mateusz-sekara Sep 11, 2023

mateusz-sekara Sep 18, 2023

dimkouv Sep 7, 2023

RensR Sep 12, 2023

RensR Sep 20, 2023

dimkouv Sep 20, 2023

dimkouv Sep 20, 2023

dimkouv Sep 7, 2023

RensR Sep 12, 2023

connorwstein Sep 19, 2023

RensR Sep 20, 2023

dimkouv Sep 7, 2023

dimkouv Sep 7, 2023

dimkouv Sep 7, 2023

dimkouv Sep 7, 2023

RensR Sep 20, 2023

mateusz-sekara Sep 11, 2023

RensR Sep 12, 2023

mateusz-sekara Sep 11, 2023

mateusz-sekara Sep 11, 2023

mateusz-sekara Sep 11, 2023

connorwstein Sep 19, 2023

RensR Sep 20, 2023

connorwstein Sep 19, 2023

dimkouv Sep 20, 2023

dimkouv Sep 20, 2023

dimkouv Sep 20, 2023

RensR Sep 20, 2023

connorwstein Sep 20, 2023

RensR Sep 20, 2023


		tokenDataProviders := make(map[common.Address]offchaintokendata.Provider)

		// TODO the called function only uses the chainId to get the message transmitter address

Offchain usdc #112

Offchain usdc #112

Conversation

RensR commented Sep 5, 2023 • edited

Motivation

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RensR commented Sep 5, 2023 •

edited