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-5233] CCIP: price-only commit report method override. #16422

Open

winder wants to merge 7 commits into develop from will/price-only-transmit

+87 −62

Contributor

winder commented Feb 14, 2025 •

edited

Loading

Adds option to override the commit method for price-only commit reports.

Implemented by extending the ToExecCalldata handler to return the contract/method in addition to the args object.

winder requested a review from a team as a code owner

February 14, 2025 22:02

winder requested review from asoliman92, makramkd, dimkouv, mateusz-sekara, 0xAustinWang, 0xnogo and b-gopalswami

February 14, 2025 22:02

Contributor

github-actions bot commented Feb 14, 2025 •

edited

Loading

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

#added For any new functionality added.
#breaking_change For any functionality that requires manual action for the node to boot.
#bugfix For bug fixes.
#changed For any change to the existing functionality.
#db_update For any feature that introduces updates to database schema.
#deprecation_notice For any upcoming deprecation functionality.
#internal For changesets that need to be excluded from the final changelog.
#nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
#removed For any functionality/config that is removed.
#updated For any functionality that is updated.
#wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

Contributor

github-actions bot commented Feb 14, 2025 •

edited

Loading

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

winder force-pushed the will/price-only-transmit branch from 22d4b5b to 6dc66cf Compare

February 15, 2025 22:46

archseer reviewed

View reviewed changes

core/capabilities/ccip/ocrimpls/contract_transmitter.go Outdated Show resolved Hide resolved

archseer previously approved these changes

View reviewed changes

Contributor

archseer left a comment

Validated on the e2e testing branch

winder changed the title ~~CCIP: price-only commit report method override.~~ [CCIP-5233] CCIP: price-only commit report method override.

makramkd reviewed

View reviewed changes

core/capabilities/ccip/ocrimpls/contract_transmitter.go Outdated

    
            @@ -23,54 +23,63 @@ type ToCalldataFunc func(
          
              	report ocr3types.ReportWithInfo[[]byte],

              	rs, ss [][32]byte,

              	vs [32]byte,

              ) (any, error)

              ) (string, string, any, error)

Contributor

makramkd Feb 18, 2025

Lets add names to the return vars to self-document

Contributor

dimkouv Feb 19, 2025

we can return a struct instead

core/capabilities/ccip/ocrimpls/contract_transmitter.go Outdated

    
              ) (any, error)

              ) (string, string, any, error)

              func NewToCommitCalldata(defaultMethod, priceOnlyMethod string) ToCalldataFunc {

Contributor

makramkd Feb 18, 2025

Godoc on this would be good to have, to explain its existence vs. lack of one for exec.

Contributor

makramkd Feb 18, 2025

Also NewToCommitCalldataFunc seems to be a better name given the ret val

core/capabilities/ccip/ocrimpls/contract_transmitter.go Outdated

    
              		if err != nil {

              			return nil, err

              		method := defaultMethod

              		if len(priceOnlyMethod) > 0 && len(info.MerkleRoots) == 0 && len(info.TokenPrices) > 0 {

Contributor

makramkd Feb 18, 2025

Doing len(string) seems awkward/non-idiomatic, I feel like priceOnlyMethod != "" reads better.

Also, if we always require a merkle root then shouldn't len(info.MerkleRoots) == 1? cc @archseer

Contributor

makramkd Feb 18, 2025

Just making a note that you can't tweak this array here since it'll fail the sig check onchain, this has to come from the plugin.

Contributor Author

winder Feb 18, 2025 •

edited

Loading

Presumably it's invalid to have a report where len(info.MerkleRoots) == 0 && len(info.TokenPrices) == 0.

onchain method	HasRoot(s)	HasPrices
commit	true	true
commit	true	false
price_only_commit	false	true
N/A	false	false

I think the report codec should be responsible for detect if the report was invalid not the transmitter. So I'm proposing this code ignores the N/A case above.

core/capabilities/ccip/ocrimpls/contract_transmitter.go

Comment on lines -214 to +233

    
              	if err := c.cw.SubmitTransaction(ctx, c.contractName, c.method, args, fmt.Sprintf("%s-%s-%s", c.contractName, c.offrampAddress, txID.String()), c.offrampAddress, &meta, zero); err != nil {

              	if err := c.cw.SubmitTransaction(ctx, contract, method, args, fmt.Sprintf("%s-%s-%s", contract, c.offrampAddress, txID.String()), c.offrampAddress, &meta, zero); err != nil {

Contributor

makramkd Feb 18, 2025

Can't comment on the line in this PR but there is still some evm-specific signature splitting above (evmutil.SplitSignature) that may not work for Solana.

Contributor Author

winder Feb 18, 2025

ah, damn. Thanks for pointing that out.

Contributor Author

winder Feb 19, 2025

On further investigation, evmutil.SplitSignature is just poorly named. It should be named ecdsautil.SplitSignature. It's unlikely that we'd use a different signature for OCR reports, so this is not something that would change now or in the future.

core/capabilities/ccip/ocrimpls/contract_transmitter_test.go Outdated

    
            @@ -316,7 +317,7 @@ func newTestUniverse(t *testing.T, ks *keyringsAndSigners[[]byte]) *testUniverse
          
              		contractName,

              		methodTransmitWithSignatures,

              		ocr3HelperAddr.Hex(),

              		ocrimpls.ToCommitCalldata,

              		ocrimpls.NewToCommitCalldata(consts.MethodCommit, ""),

Contributor

makramkd Feb 18, 2025

Can we have a test for the Solana transmission?

Contributor Author

winder Feb 19, 2025

The Solana e2e test would cover this. I'm not sure how a unit test would work.

core/capabilities/ccip/oraclecreator/plugin.go

Comment on lines +102 to +98

    
              	// PriceOnlyCommitFn optional method override for price only commit reports.

              	PriceOnlyCommitFn string

Contributor

makramkd Feb 18, 2025

Where/how would this get set?

Contributor Author

winder Feb 18, 2025

It's set a few lines above. Hard coded for the Solana chain family.

Contributor Author

winder commented Feb 19, 2025

@makramkd @dimkouv created a ticket for future refactoring:
https://smartcontract-it.atlassian.net/browse/CCIP-5276

matYang previously approved these changes

View reviewed changes

winder dismissed stale reviews from matYang and archseer via

3a02939

February 25, 2025 13:00

winder force-pushed the will/price-only-transmit branch from 926456a to 9c75cee Compare

February 25, 2025 21:23

winder added 5 commits

February 26, 2025 10:13


          Option to send price only commit reports to a different method.

82c9989


          Update test compilation.

01b2ffc


          Lint

36039db


          PR feedback.

cfe4119


          Add named params.

7182d62

winder force-pushed the will/price-only-transmit branch from 9c75cee to 7182d62 Compare

February 26, 2025 15:15


          Add missing import.

makramkd approved these changes

View reviewed changes

Contributor

makramkd commented Feb 26, 2025

Can you merge in latest develop so the in-memory tests can run as well? There was a hiccup recently #16586


          Merge remote-tracking branch 'origin/develop' into will/price-only-tr…

70faf30

…ansmit

archseer approved these changes

View reviewed changes

archseer enabled auto-merge

February 27, 2025 05:43

archseer added this pull request to the merge queue

Any commits made after this event will not be merged.

cl-sonarqube-production bot commented Feb 27, 2025

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

archseer archseer approved these changes

makramkd makramkd approved these changes

matYang matYang left review comments

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

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

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

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

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

b-gopalswami Awaiting requested review from b-gopalswami b-gopalswami is a code owner automatically assigned from smartcontractkit/ccip-offchain

Labels

None yet