Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CCIP-5109 LBTC tokendata #558

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

CCIP-5109 LBTC tokendata #558

wants to merge 12 commits into from

Conversation

bukata-sa
Copy link

@bukata-sa bukata-sa commented Feb 5, 2025 •
edited
Loading

core ref: f5957b8389a5968df275eba3918dfe941dac525c

This PR proposes tokendata refactoring + LBTC tokendata enablement

Config refactoring

  1. Split config parts into different structs, embed them and provide custom un/marshalling functions - existing configs are still compatible with these changes
  2. Moving observer to separated package for dependency management
  3. Moving http client to separated package and make it generic for both LBTC and USDC. Although it still depends on usage: POST action for LBTC and GET action for USDC
  4. Moving common (USDC and LBTC) parts to token_data.go file in the root of tokendata package

LBTC attestation

One-pager: https://smartcontract-it.atlassian.net/wiki/spaces/CCIP/pages/1204256885/Lombard+LBTC+Attestation

  1. Observer extracts ExtraData from tokenAmount
  2. If ExtraData is not 32-length byte array - skip attestation
  3. Otherwise call Lombard Attestation API using API provided by configuration
  4. Attestation response if found represents ready byte array that need to passed as offchain tokendata and will be accepted by LBTC contract
  5. If attestation is not found, return an error (token data not ready for example)

This PR contains unit and component tests, but lacks integration tests in core repo

@bukata-sa bukata-sa requested a review from a team as a code owner February 5, 2025 07:57
@bukata-sa bukata-sa force-pushed the feature/tokendata_lbtc branch 10 times, most recently from 6b0ed0c to 5b8286d Compare February 7, 2025 15:26
@bukata-sa bukata-sa force-pushed the feature/tokendata_lbtc branch 11 times, most recently from 47c2c5e to e19c13e Compare February 19, 2025 10:53
@bukata-sa bukata-sa mentioned this pull request Feb 19, 2025
Open
@bukata-sa bukata-sa force-pushed the feature/tokendata_lbtc branch from 76567fa to c9f1900 Compare February 19, 2025 12:20
dimkouv
dimkouv reviewed Feb 21, 2025
View reviewed changes
Copy link
Contributor

@dimkouv dimkouv left a comment

Choose a reason for hiding this comment

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

Looks good but kinda hard to properly review due to the PR size, I recommend making smaller individual PRs in the future.

e.g.

  1. refactoring pr / rename packages / etc
  2. add lbtc token data reader
  3. start using lbtc token data reader

pluginconfig/token.go Outdated
if err != nil {
return nil, err
}
return json.Marshal(raw)
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't there a more efficient way? i see we do marshal -> unmarshal -> marshal

Copy link
Author

Choose a reason for hiding this comment

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

hope having anonymus struct is more efficient than marshal -> unmarshal -> marshal

pluginconfig/token.go
// Default to 100 millis if AttestationAPIInterval is not set this is set according to the APIs documented
// 10 requests per second rate limit.
if p.AttestationAPIInterval == nil {
p.AttestationAPIInterval = commonconfig.MustNewDuration(100 * time.Millisecond)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure all this are configurable

Copy link
Author

@bukata-sa bukata-sa Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What do you mean? I have updated all tests with this configuration
I also added unmarshal test

chainlink-ccip/pluginconfig/token_test.go

Line 17 in f0861cc

func Test_TokenDataObserver_Unmarshal(t *testing.T) {
to make sure that configs unmarshals properly (as they come from capability)

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean we should be able to configure all this parameter without having to make a new release.

@bukata-sa bukata-sa force-pushed the feature/tokendata_lbtc branch 2 times, most recently from d295948 to f0861cc Compare February 24, 2025 08:49
@bukata-sa bukata-sa force-pushed the feature/tokendata_lbtc branch from f0861cc to 729ebb5 Compare February 24, 2025 08:53
@bukata-sa bukata-sa requested a review from dimkouv February 24, 2025 10:31
@github-actions GitHub Actions
Copy link

Metric feature/tokendata_lbtc main
Coverage 74.6% 74.1%

dimkouv
dimkouv approved these changes Feb 25, 2025
View reviewed changes
Copy link
Contributor

@dimkouv dimkouv left a comment

Choose a reason for hiding this comment

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

Approving to unblock, but I haven’t done a full deep dive on every detail due to the size of the PR. Please make sure to get additional reviews and thoroughly test the changes (including proper E2E validation) before merging.

pluginconfig/token.go
// Default to 100 millis if AttestationAPIInterval is not set this is set according to the APIs documented
// 10 requests per second rate limit.
if p.AttestationAPIInterval == nil {
p.AttestationAPIInterval = commonconfig.MustNewDuration(100 * time.Millisecond)
Copy link
Contributor

Choose a reason for hiding this comment

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

I mean we should be able to configure all this parameter without having to make a new release.

@bukata-sa bukata-sa marked this pull request as draft February 26, 2025 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimkouv dimkouv dimkouv approved these changes

@winder winder Awaiting requested review from winder winder is a code owner automatically assigned from smartcontractkit/ccip-offchain

@asoliman92 asoliman92 Awaiting requested review from asoliman92 asoliman92 is a code owner automatically assigned from smartcontractkit/ccip-offchain

@makramkd makramkd Awaiting requested review from makramkd makramkd is a code owner automatically assigned from smartcontractkit/ccip-offchain

@mateusz-sekara mateusz-sekara Awaiting requested review from mateusz-sekara mateusz-sekara is a code owner automatically assigned from smartcontractkit/ccip-offchain

@0xAustinWang 0xAustinWang Awaiting requested review from 0xAustinWang 0xAustinWang is a code owner automatically assigned from smartcontractkit/ccip-offchain

@0xnogo 0xnogo Awaiting requested review from 0xnogo 0xnogo is a code owner automatically assigned from smartcontractkit/ccip-offchain

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bukata-sa @dimkouv