CCIP-5109 Add LBTC tokendata #801
Conversation
bukata-sa
requested review from
winder,
asoliman92,
matYang,
makramkd,
dimkouv,
0xAustinWang and
0xnogo
bukata-sa
force-pushed
the
feature/tokendata_add_lbtc
branch
from
b0b9439 to
879c8a3
Compare
| @@ -39,6 +40,7 @@ type HTTPClient interface { | |||
| // https://developers.circle.com/stablecoins/reference/getattestation | |||
| // https://developers.circle.com/stablecoins/docs/transfer-usdc-on-testnet-from-ethereum-to-avalanche | |||
| Get(ctx context.Context, path string) (cciptypes.Bytes, HTTPStatus, error) | |||
| Post(ctx context.Context, path string, requestData cciptypes.Bytes) (cciptypes.Bytes, HTTPStatus, error) | |||
There was a problem hiding this comment.
Should this httpClient be part of tokenData/ package? isn't it a generic http lib? maybe as a followup / create a ticket to move it.
There was a problem hiding this comment.
This client is still coupled to attestation calls. More than that, Get - for USDC, Post - for LBTC
Need additional refactoring to generalize it away from token data
| @@ -130,6 +132,27 @@ func (h *httpClient) Get(ctx context.Context, requestPath string) (cciptypes.Byt | |||
| return response, httpStatus, err | |||
| } | |||
|
|
|||
| func (h *httpClient) Post( | |||
There was a problem hiding this comment.
general note: Would be nice to create separate PRs (e.g. to introduce Post/ in httpClient) to reduce review scope.
| attestationStatusFailed AttestationStatus = "NOTARIZATION_STATUS_FAILED" | ||
| attestationStatusPending AttestationStatus = "NOTARIZATION_STATUS_PENDING" | ||
| attestationStatusSubmitted AttestationStatus = "NOTARIZATION_STATUS_SUBMITTED" | ||
| _ AttestationStatus = "NOTARIZATION_STATUS_SESSION_APPROVED" |
There was a problem hiding this comment.
Is this blank because compiler complains that it's not used?
Maybe include a link to docs here? and mention that there are more status that we don't use
There was a problem hiding this comment.
I'm not aware of any Lombard documentation with this API
With this const list, we have all statuses that Lombard produces, removing it from here brings uncertainty, don't you agree?
pluginconfig/execute.go
Outdated
| @@ -106,6 +106,19 @@ func (e *ExecuteOffchainConfig) IsUSDCEnabled() bool { | |||
| return false | |||
| } | |||
|
|
|||
| func (e *ExecuteOffchainConfig) IsLBTCEnabled() bool { | |||
| for _, ob := range e.TokenDataObservers { | |||
| if ob.WellFormed() != nil { | |||
There was a problem hiding this comment.
nit: maybe more simple to do:
if ob.WellFormed() != nil && ob.IsLBTC() {
return true
}
There was a problem hiding this comment.
fixed, but it should be
if ob.WellFormed() == nil && ob.IsLBTC() {
return true
}
|
Is there a plan for e2e integration tests? can we have e2e integ tests? |
bukata-sa
force-pushed
the
feature/tokendata_add_lbtc
branch
2 times, most recently
from
e25ece3 to
73de19a
Compare
bukata-sa
force-pushed
the
feature/tokendata_add_lbtc
branch
from
73de19a to
fde9ae3
Compare
|
|
No description provided.