Adds deployment labels on Forwarder deployment #16653
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌35 Results
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌35 Results
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌9 Results
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌9 Results
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌6 Results
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs-02ce8d70-9a8f-45d9-a8ff-1868363e23c2.json. |
vyzaldysanchez
requested review from
winder,
asoliman92,
matYang,
makramkd,
dimkouv,
mateusz-sekara,
b-gopalswami and
bolekk
…one-forwarder-deployment-lbls
3 tasks
krehermann
reviewed
Mar 5, 2025
krehermann
reviewed
Mar 5, 2025
krehermann
reviewed
Mar 5, 2025
| @@ -131,8 +131,23 @@ func GetContractSets(lggr logger.Logger, req *GetContractSetsRequest) (*GetContr | |||
| return nil, fmt.Errorf("failed to get addresses for chain %d: %w", id, err) | |||
| } | |||
|
|
|||
| // Forwarder addresses now have informative labels, but we don't want them to be ignored if no labels are provided for filtering. | |||
| // If labels are provided, just filter by those. | |||
| forwarderAddrs := make(map[string]deployment.TypeAndVersion) | |||
There was a problem hiding this comment.
i'm confused by this. can we add some tests?
i think line 145 should 'just do this'. if there are no labels, then 145 should return everything. if there are labels, then it will filter them.
why are forwarder addrs being treated so differently?
There was a problem hiding this comment.
Because they have labels, line 145 is keeping them out since we are not passing any labels on req.Labels... and if we pass any, it will just return what's specified on the labels and keep the other addresses out of it.
This was added here: #16464
There was a problem hiding this comment.
To clarify, not passing labels keep addresses with labels out of the resulting slice, if that makes sense.
There was a problem hiding this comment.
ok, let's refactor this into two different cases. if the input Labels are empty then set filtered = deployment.LabeledAddresses
and if they are not empty, do filtered.LabeledAddresses(addrs).And(...)
that will maintain the current behavior while also supporting non-empty input labels correctly
There was a problem hiding this comment.
@krehermann this is the same as it was before this change. We need to then on each call in which we want all contract sets (unlabeled and forwarders), for example to call TransferableContracts(), we need to call GetContractSets() twice: one without input labels and one with input labels. Because passing labels only return the addresses that match ALL labels, not SOME labels.
So taking that into consideration, it would be one call for all unlabeled contracts, and X calls for X forwarder contract address...
This is why I went with keeping the forwarder addresses here since the label filtering doesn't take into consideration the fact that we may have informational labeled addresses, and that we would like to bring those each time, regardless on whether we pass a labels input or not.
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌1 Results
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs-0a04c0a2-cd22-4f02-a7ad-5864338e2aa7.json. |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses the first part of https://smartcontract-it.atlassian.net/browse/CRE-338, which consists of tracking deployment information of the Forwarder contract as part of the
TypeAndVersionlabels.This deployment info consists of block number and transaction hash. We want to use this info to later fetch
SetConfigevents to generate the view in a more performant way.Related PRs
ForwarderViewforKeystoneView" #16637 -> this will be re-added after validating that the labels approach is OK.Requires
Supports