CCIP-2882 Implementing attestation API #162

mateusz-sekara · 2024-09-25T08:14:48Z

No description provided.

mateusz-sekara · 2024-09-25T08:16:23Z

pluginconfig/execute.go

@@ -90,136 +89,6 @@ func (e ExecuteOffchainConfig) Validate() error {
 	return nil
 }

-// TokenDataObserverConfig is the base struct for token data observers. Every token data observer


Extracted to a separate file, no changes

mateusz-sekara · 2024-09-25T08:17:03Z

pluginconfig/execute_test.go

@@ -184,274 +181,3 @@ func TestExecuteOffchainConfig_EncodeDecode(t *testing.T) {
 		})
 	}
 }
-
-func Test_TokenDataObserver_Unmarshall(t *testing.T) {


Extracted to a separate file, no changes

mateusz-sekara · 2024-09-25T08:17:17Z

pluginconfig/token.go

+// in the TokenDataObserverConfig similarly to how USDCCCTPObserverConfig is embedded in the TokenDataObserverConfig.
+// There are two additional checks for the TokenDataObserverConfig to enforce that it's semantically (Validate)
+// and syntactically correct (WellFormed).
+type TokenDataObserverConfig struct {


Copied from exectypes, no changes here

mateusz-sekara · 2024-09-25T08:17:21Z

pluginconfig/token_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func Test_TokenDataObserver_Unmarshall(t *testing.T) {


Copied from exectypes, no changes here

mateusz-sekara · 2024-09-25T08:47:24Z

execute/tokendata/usdc/attestation.go

+		outcome[chainSelector] = make(map[exectypes.MessageTokenID]AttestationStatus)
+
+		for tokenID, messageHash := range hashes {
+			// TODO sequential processing


Next PR, I'm optimizing for having working usdc flow asap. We can optimize the code by caching or parallel processing later

makramkd · 2024-09-25T11:35:42Z

execute/tokendata/usdc/http.go

+		}
+	}
+
+	timeoutCtx, cancel := context.WithTimeoutCause(ctx, h.apiTimeout, ErrTimeout)


q: if ctx is canceled before the timeout, does timeoutCtx cancel?

have you checked the tests? :P yeah, it cancels

makramkd · 2024-09-25T11:36:58Z

execute/tokendata/usdc/http.go

+			return response, http.StatusRequestTimeout, ErrTimeout
+		}
+		// On error, res is nil in most cases, do not read res.StatusCode, return BadRequest
+		return response, http.StatusBadRequest, err


Should we just return nil here instead of response to ensure its never accessed? I think we had a bug around this in <V1.5

response is not an http object, it's just a payload, no risk

execute/tokendata/usdc/http.go

makramkd · 2024-09-25T11:41:01Z

execute/tokendata/usdc/http.go

+
+func (h *httpClient) setCoolDownPeriod(headers http.Header) {
+	coolDownDuration := defaultCoolDownDuration
+	if retryAfterHeader, exists := headers["Retry-After"]; exists && len(retryAfterHeader) > 0 {


consider: Retry-After as a constant

not sure if worth it, it's used in a single place and it's rather obvious what it is ;)

makramkd · 2024-09-25T11:44:10Z

execute/tokendata/usdc/http.go

+//nolint:revive
+func NewHTTPClient(api string, apiInterval time.Duration, apiTimeout time.Duration) *httpClient {
+	return &httpClient{
+		apiURL:     api,


q: Do we wanna do some kind of parsing on the passed in URL so that we don't construct garbage when doing the fmt.Sprintf below?

yeah, probably makes sense to be defensive here

added parsing during object construction and tests

github-actions · 2024-09-25T12:16:22Z

Test Coverage

Branch	Coverage
attestation-api	72.6%
main	72.0%

dimkouv · 2024-09-25T12:34:30Z

execute/tokendata/observer.go

@@ -60,7 +60,7 @@ func NewConfigBasedCompositeObservers(
 		// e.g. observers[i] := config.CreateTokenDataObserver()
 		switch {
 		case c.USDCCCTPObserverConfig != nil:
-			observer, err1 := createUSDCTokenObserver(lggr, destChainSelector, c.USDCCCTPObserverConfig.Tokens, readers)
+			observer, err1 := createUSDCTokenObserver(lggr, destChainSelector, *c.USDCCCTPObserverConfig, readers)


Why err1 and not err ?

dimkouv · 2024-09-25T12:35:38Z

execute/tokendata/observer.go

@@ -60,7 +60,7 @@ func NewConfigBasedCompositeObservers(
 		// e.g. observers[i] := config.CreateTokenDataObserver()


NewConfigBasedCompositeObservers is an exported function that returns an unexported type. In general it's not a good approach. Either return interface or exported struct.

dimkouv · 2024-09-25T12:37:42Z

execute/tokendata/usdc/attestation.go

 )

 type AttestationStatus struct {
+	// MessageHash is the hash of the message that the attestation was fetched for. It's going to be MessageSent event hash
+	MessageHash []byte


You can replace []byte with cciptypes.Bytes, that way you also get cool hex encoding out of the box in logs.

dimkouv · 2024-09-25T12:38:50Z

execute/tokendata/usdc/attestation.go

+	ErrNotReady        = errors.New("token data not ready")
+	ErrRateLimit       = errors.New("token data API is being rate limited")
+	ErrTimeout         = errors.New("token data API timed out")
+	ErrUnknownResponse = errors.New("unexpected response from attestation API")
 )

 type AttestationStatus struct {


Would be nice to have comment for AttestationStatus.
e.g.

// AttestationStatus is returned from CCTP api / is used internally to keep track of / ...

dimkouv · 2024-09-25T12:40:47Z

execute/tokendata/usdc/attestation.go

@@ -30,15 +45,62 @@ type AttestationStatus struct {
 // Ethereum ->
 //
 //	12 ->


Does this mean for 12 there is no entry in the map?

It's nested map :D So 12 is a chainSelector, next line contains padded messageTokenIds

dimkouv · 2024-09-25T12:52:35Z

execute/tokendata/usdc/http.go

+func (r httpResponse) attestationToBytes() ([]byte, error) {
+	attestationBytes, err := hex.DecodeString(strings.TrimPrefix(r.Attestation, "0x"))
+	if err != nil {
+		return nil, err


dimkouv · 2024-09-25T12:53:34Z

execute/tokendata/usdc/http.go

+	defer cancel()
+
+	requestURL := *h.apiURL
+	requestURL.Path = path.Join(requestURL.Path, apiVersion, attestationPath, fmt.Sprintf("0x%x", messageHash))


can replace fmt.Sprintf("0x%x"..... with cciptypes.Bytes(messageHash).String()

dimkouv · 2024-09-25T12:54:33Z

execute/tokendata/usdc/http.go

+	// failing to make any progress.
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, url.String(), nil)
+	if err != nil {
+		return response, http.StatusBadRequest, err


dimkouv · 2024-09-25T12:57:04Z

execute/tokendata/usdc/http.go

+		return nil, res.StatusCode, fmt.Errorf("failed to decode json: %w", err)
+	}
+
+	if err1 := response.validate(); err1 != nil {


i think err1 can be just err

dimkouv · 2024-09-25T12:57:55Z

execute/tokendata/usdc/http.go

+		return nil, res.StatusCode, err1
+	}
+
+	attestationBytes, err := response.attestationToBytes()


maybe better to wrap this err

if err != nil { return fmt.Errorf("convert attestation to bytes: %w") return bytes, code, nil

dimkouv · 2024-09-25T13:00:41Z

execute/tokendata/usdc/http.go

+	defaultCoolDownDuration = 5 * time.Minute
+
+	// maxCoolDownDuration defines the maximum duration we can wait till firing the next request
+	maxCoolDownDuration = 10 * time.Minute


Let's have this configurable, provide them in the constructor.

dimkouv · 2024-09-25T13:02:20Z

go.mod

@@ -12,6 +12,7 @@ require (
 	golang.org/x/crypto v0.27.0
 	golang.org/x/exp v0.0.0-20240909161429-701f63a606c0
 	golang.org/x/sync v0.8.0
+	golang.org/x/time v0.3.0


We want this for ratelimiting?

Yeah, this is exactly how it's done for v1.5, let's keep it as it

0xnogo · 2024-09-25T12:18:34Z

execute/tokendata/usdc/http.go

+func (h *httpClient) setCoolDownPeriod(headers http.Header) {
+	coolDownDuration := defaultCoolDownDuration
+	if retryAfterHeader, exists := headers["Retry-After"]; exists && len(retryAfterHeader) > 0 {
+		if retryAfterSec, errParseInt := strconv.ParseInt(retryAfterHeader[0], 10, 64); errParseInt == nil {


Is it always an array of size 1? Wondering if we know what could be in the array?

0xnogo · 2024-09-25T12:24:46Z

execute/tokendata/usdc/http.go

+
+	h.coolDownMu.Lock()
+	defer h.coolDownMu.Unlock()
+	coolDownDuration = min(coolDownDuration, maxCoolDownDuration)


Does it mean that if Circle told us to cooldown for >10m we still going to fire requests? Why not just relying on coolDownDuration?

0xnogo · 2024-09-25T12:24:49Z

execute/tokendata/usdc/http.go

+func (h *httpClient) setCoolDownPeriod(headers http.Header) {
+	coolDownDuration := defaultCoolDownDuration
+	if retryAfterHeader, exists := headers["Retry-After"]; exists && len(retryAfterHeader) > 0 {
+		if retryAfterSec, errParseInt := strconv.ParseInt(retryAfterHeader[0], 10, 64); errParseInt == nil {


Is it always an array of size 1? Wondering if we know what could be in the array?

0xnogo · 2024-09-25T12:31:39Z

execute/tokendata/usdc/http.go

+		// Wait blocks until it the attestation API can be called or the
+		// context is Done.
+		if waitErr := h.rate.Wait(ctx); waitErr != nil {
+			return empty, http.StatusTooManyRequests, ErrRateLimit


Wondering if we want to differentiate the 2 err. Something like ErrRateLimit and ErrCoolDown

Implementing attestation API

fe28052

mateusz-sekara commented Sep 25, 2024

View reviewed changes

mateusz-sekara added 3 commits September 25, 2024 10:32

Minor fixes

d0537e0

Minor fixes

f11d0f5

Merge branch 'main' into attestation-api

c535564

mateusz-sekara marked this pull request as ready for review September 25, 2024 08:44

mateusz-sekara requested a review from a team as a code owner September 25, 2024 08:44

mateusz-sekara changed the title ~~Implementing attestation API~~ CCIP-2882 Implementing attestation API Sep 25, 2024

mateusz-sekara commented Sep 25, 2024

View reviewed changes

Rate limiter tests from chainlink repo

c112c23

makramkd reviewed Sep 25, 2024

View reviewed changes

execute/tokendata/usdc/http.go Outdated Show resolved Hide resolved

makramkd reviewed Sep 25, 2024

View reviewed changes

makramkd approved these changes Sep 25, 2024

View reviewed changes

mateusz-sekara added 2 commits September 25, 2024 13:50

Post review fixes

9e21625

Post review fixes

6ae1f66

mateusz-sekara force-pushed the attestation-api branch from 30e8ecf to 6ae1f66 Compare September 25, 2024 12:13

dimkouv reviewed Sep 25, 2024

View reviewed changes

mateusz-sekara merged commit 7eba4f9 into main Sep 25, 2024
3 checks passed

mateusz-sekara deleted the attestation-api branch September 25, 2024 13:51

0xnogo reviewed Sep 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCIP-2882 Implementing attestation API #162

CCIP-2882 Implementing attestation API #162

mateusz-sekara commented Sep 25, 2024

mateusz-sekara Sep 25, 2024 •

edited

Loading

mateusz-sekara Sep 25, 2024

mateusz-sekara Sep 25, 2024

mateusz-sekara Sep 25, 2024

mateusz-sekara Sep 25, 2024

makramkd Sep 25, 2024

mateusz-sekara Sep 25, 2024

makramkd Sep 25, 2024

mateusz-sekara Sep 25, 2024

makramkd Sep 25, 2024

mateusz-sekara Sep 25, 2024

makramkd Sep 25, 2024

mateusz-sekara Sep 25, 2024

mateusz-sekara Sep 25, 2024

github-actions bot commented Sep 25, 2024

dimkouv Sep 25, 2024

dimkouv Sep 25, 2024

dimkouv Sep 25, 2024

dimkouv Sep 25, 2024

dimkouv Sep 25, 2024

mateusz-sekara Sep 25, 2024

dimkouv Sep 25, 2024

dimkouv Sep 25, 2024

dimkouv Sep 25, 2024

dimkouv Sep 25, 2024

dimkouv Sep 25, 2024

dimkouv Sep 25, 2024

dimkouv Sep 25, 2024

mateusz-sekara Sep 25, 2024

0xnogo Sep 25, 2024

0xnogo Sep 25, 2024

0xnogo Sep 25, 2024

0xnogo Sep 25, 2024

		@@ -60,7 +60,7 @@ func NewConfigBasedCompositeObservers(
		// e.g. observers[i] := config.CreateTokenDataObserver()

CCIP-2882 Implementing attestation API #162

CCIP-2882 Implementing attestation API #162

Conversation

mateusz-sekara commented Sep 25, 2024

mateusz-sekara Sep 25, 2024 • edited Loading

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

github-actions bot commented Sep 25, 2024

Test Coverage

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

mateusz-sekara Sep 25, 2024 •

edited

Loading