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.

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

[DON'T MERGE] CCIP Repository Merge - ocr2 #13975

Closed
wants to merge 358 commits into from

Conversation

asoliman92
Copy link
Contributor

@asoliman92 asoliman92 commented Jul 31, 2024

DO NOT MERGE THAT PR, once approved and working we need to figure out way to merge it instead of squashing. Otherwise, entire effort on preserving commits history from CCIP repo will be lost.

Next steps:

  • ccip eng review
  • core eng review
  • figuring out strategy for merge instead of squash

mateusz-sekara and others added 30 commits November 21, 2023 11:22
merge core develop -> ccip-develop

fix conflicts, build

use openzeppelin-solidity v4.8.3

add ERC165Checker.sol to the vendor, wasn't there before

add missing file

update shared snapshot

update gethwrappers

add ERC165.sol to fix metatx compile

update generated files

update modgraph

fix one config test

fix some config tests

fix config again

more config test fixes

bump mockery version in contracts makefile

go.mod updates

more updates

remove integration-tests dependencies

USDC filter fix (#383)

fix integration-tests go mod

more fixes

fix lint errors

fix TestSecrets_Validate

fix TestResolver_ConfigV2

fix TestIntegration_CCIP

try using crypto/rand

remove build-publish-pr.yml

skip metatx test
fix integration-tests/go.mod

fix imports
@mateusz-sekara mateusz-sekara changed the title Ccip/merge ocr2 CCIP Repository Merge - ocr2 Aug 2, 2024
@mateusz-sekara mateusz-sekara marked this pull request as ready for review August 2, 2024 10:16
@mateusz-sekara mateusz-sekara requested review from a team as code owners August 2, 2024 10:17
@mateusz-sekara mateusz-sekara changed the title CCIP Repository Merge - ocr2 DONCCIP Repository Merge - ocr2 Aug 2, 2024
@mateusz-sekara mateusz-sekara changed the title DONCCIP Repository Merge - ocr2 [DON'T MERGE] CCIP Repository Merge - ocr2 Aug 2, 2024
@@ -107,6 +107,9 @@ type Service interface {
ListSpecsByJobProposalIDs(ctx context.Context, ids []int64) ([]JobProposalSpec, error)
RejectSpec(ctx context.Context, id int64) error
UpdateSpecDefinition(ctx context.Context, id int64, spec string) error

// UnsafeSetConnectionsManager Only for testing
UnsafeSetConnectionsManager(ConnectionsManager)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@asoliman92 Have you checked if it's still needed? Locally integration_tests.go and integration_legacy_test.go works for me when removed

Checking here smartcontractkit/ccip#1253

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ignore that, it's needed for CLO tests I created long time ago :P (Test_CLOSpecApprovalFlow)

if err2 != nil {
d.lggr.Errorw("failed to unregister ccip exec plugin filters", "err", err2, "spec", spec)
}
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this logic live in cleanupEVM as opposed to triggering when a service is closed?

var multiErr error
for _, fn := range unregisterFuncs {
if err := fn(); err != nil {
multiErr = multierr.Append(multiErr, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

We've been trying to drop this import in favor of the std lib:

Suggested change
multiErr = multierr.Append(multiErr, err)
multiErr = errors.Join(multiErr, err)

That can be a follow up change though.

}
}

return allStatuses, counter - 1, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Why subtract one? Returning from here means the break on 42 was encountered, before incrementing on 45. Isn't counter accurate as-is?

@winder
Copy link
Contributor

winder commented Aug 2, 2024

To review this we should start with the following:

  1. A script/algorithm for how the files were moved to chainlink. What was the original commit on the ccip repo?
  2. A script to diff the source ccip commit files and the dest chainlink files.
  3. Targeted review on the diff between source and dest.

On a quick inspection it looks like about 1/3 of the files were modified during the migration:

#!/usr/bin/env bash

if [[ "$OSTYPE" == "darwin"* ]]; then
    # macOS
    SUM=md5
else
    # Assume Linux
    SUM=md5sum
fi

pushd ccip/core/services/ocr2/plugins/ccip/ > /dev/null
echo -e "ccip repo commit: \n$(git log --max-count=1 --oneline --decorate)"
find . -type f -exec $SUM {} \;  > ../files.md5
popd > /dev/null
mv ./ccip/core/services/ocr2/plugins/files.md5 ./chainlink/core/services/ocr2/plugins/files.md5
pushd ./chainlink/core/services/ocr2/plugins/ccip/ > /dev/null
echo -e "chainlink repo commit: \n$(git log --max-count=1 --oneline --decorate)"

echo ""

echo "Source files not modified:"
$SUM -c ../files.md5 2>/dev/null | grep OK | wc -l
echo "Source files modified:"
$SUM -c ../files.md5 2>/dev/null | grep -v OK | wc -l
ccip repo commit:
1da6ea640d (HEAD -> ccip-develop) Cleanup http test servers (#1163)
chainlink repo commit:
01a5830252 (HEAD -> ccip/merge-ocr2, origin/ccip/merge-ocr2) Adding mockery configuration for ccip specific code

Source files not modified:
125
Source files modified:
60

asoliman92 added a commit that referenced this pull request Aug 4, 2024
This branch is to help review the move from CCIP to Chainlink repo in #13975
@asoliman92 asoliman92 changed the base branch from develop to ccip/ocr2-copy-b851bb3 August 4, 2024 14:44
@winder
Copy link
Contributor

winder commented Aug 4, 2024

The following commits are identical according to the script:

ccip: git checkout 44b23a5a32
chainlink: git checkout ccip/merge-ocr2 && git checkout b851bb3e87

@asoliman92 asoliman92 closed this Aug 7, 2024
@mateusz-sekara mateusz-sekara deleted the ccip/merge-ocr2 branch November 6, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.