New rmn curse changeset #15868
New rmn curse changeset #15868
Conversation
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
carte7000
requested review from
winder,
asoliman92,
makramkd,
dimkouv,
mateusz-sekara,
0xAustinWang and
0xnogo
| state, err := LoadOnchainState(e) | ||
|
|
||
| if err != nil { | ||
| return errors.Errorf("failed to load onchain state: %v", err) |
There was a problem hiding this comment.
nit: we're trying to actively discontinue the use of errors in favor of fmt - so here fmt.Errorf instead
There was a problem hiding this comment.
@makramkd I had to change this to make the linter pass, everything was using fmt.Errorf but linting would fail
There was a problem hiding this comment.
What is the linting error?
There was a problem hiding this comment.
I can't find the previous run, it will probably be quicker to change the format once more and see if the linting issues reappear, anyways from what I get we should probably update the linting rule if this flags fmt.Errorf
| grouped[subject.ChainSelector] = append(grouped[subject.ChainSelector], subject.SubjectToCurse) | ||
| } | ||
|
|
||
| // Only keep unique subjects, preserve only global curse if present and eliminate any curse where the selector is the same as the subject |
There was a problem hiding this comment.
eliminate any curse where the selector is the same as the subject
Trying to understand this, aren't the subjects either global curse or the chain selector? So "eliminate any curse where the selector is the same as the subject" are just non-global curses no? Might be just simpler to say 'if the global curse subject is one of the subjects, prune all other subjects and only keep the global one'?
There was a problem hiding this comment.
I think my comment was not clear, I changed it, however the goal here was to avoid self curse or in other word having a chain curse it's own chain selector on RMNRemote
| } | ||
| } | ||
|
|
||
| func groupRMNSubjectBySelector(rmnSubjects []RMNCurseAction, filter bool) map[uint64][]Subject { |
There was a problem hiding this comment.
| func groupRMNSubjectBySelector(rmnSubjects []RMNCurseAction, filter bool) map[uint64][]Subject { | |
| func groupRMNSubjectBySelector(rmnSubjects []RMNCurseAction, keepOnlyGlobal bool) map[uint64][]Subject { |
Based on my understanding of this code, this seems like a more meaningful var name, please correct me if I'm wrong though
There was a problem hiding this comment.
I changed the function to have less confusing options but keepOnlyGlobal would not be enough as we also need to remove the self curses I was mentioning above.
| } | ||
|
|
||
| func groupRMNSubjectBySelector(rmnSubjects []RMNCurseAction, filter bool) map[uint64][]Subject { | ||
| grouped := make(map[uint64][]Subject) |
There was a problem hiding this comment.
This seems like a simpler approach to me:
func groupRMNSubjectBySelector(rmnSubjects []RMNCurseAction, keepOnlyGlobal bool) (result map[uint64][]Subject) {
grouped := make(map[uint64]map[Subject]struct{})
// Group subjects by chain selector and ensure uniqueness
for _, subject := range rmnSubjects {
if grouped[subject.ChainSelector] == nil {
grouped[subject.ChainSelector] = make(map[Subject]struct{})
}
if keepOnlyGlobal && subject.SubjectToCurse == SelectorToSubject(subject.ChainSelector) {
continue
}
grouped[subject.ChainSelector][subject.SubjectToCurse] = struct{}{}
}
result = make(map[uint64][]Subject)
for selector, subjects := range grouped {
// see https://pkg.go.dev/golang.org/x/exp/maps#Values
result[selector] = maps.Values(subjects)
}
return result
}There was a problem hiding this comment.
I think this might be missing some parts, ie if the GlobalSubject is part of the subject to curse on a chain we would want to only keep this one when keepOnlyGlobal is active. I updated the original to be clearer let me know if it looks good to you
| sim := &bind.TransactOpts{ | ||
| From: txOpts.From, | ||
| Signer: txOpts.Signer, | ||
| GasLimit: txOpts.GasLimit, |
There was a problem hiding this comment.
Not 100% sure what these values would be in DeployerKey but if this is non-zero the gethwrappers won't estimate gas, see e.g transactions https://github.com/ethereum/go-ethereum/blob/c0882429f032da58620bcaa610007939aa7e0adb/accounts/abi/bind/base.go#L316
There was a problem hiding this comment.
I see but I don't think we would be modifying the original behavior from the deployerKey here no?
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
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 ❌
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 ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
Yes we have one with MCMS and one with deployer key for each Curse/Uncurse test cases |
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
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 ❌
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 ❌
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 ❌
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 ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
This PR add a new changeset to curse lanes and chains via RMNRemote
It also add a utility that allow writing changeset without caring about wether we are creating a proposal or executing with a deployer key.
Example usage of the changeset