Fix regression test path and improve logs for debugging #771
Conversation
ImJeremyHe
requested review from
Sneh1999,
alysiahuggins,
jbearer,
jjeangal,
sveitser and
zacshowa
as code owners
| // If this is encountered, it likely means a contract upgrade is happening. | ||
| // TODO: Check that all current batcher addresses are still valid after upgrade. | ||
| if bytes.Equal(data[:4], []byte{0x1c, 0xff, 0x79, 0xcd}) { | ||
| return nil, fmt.Errorf("encountering execute method, caff node needs to update") |
There was a problem hiding this comment.
Contract upgrades can happen right? I didnt understand this
During contract upgrades also I think they use setIsBatchPoster not sure
There was a problem hiding this comment.
We should handle this but not sure how we should deal with it. The monitor currently searches forOwnerFunctionCalled(1), not the event SetIsBatchPoster. So we can't get the batcher address update. I am thinking at least we can verify current addresses and remove invalid ones.
Returning error at least clearly shows what is the issue here.
There was a problem hiding this comment.
My concern is this that when a contract migration is happening it will crash all the caff nodes because it will keep throwing errors, let me know if that is not the case?
There was a problem hiding this comment.
Yes this is the case. My point is that, without this PR, this case can still happen. So this PR doesn't do anything worse and it at least shows where the error comes from
There was a problem hiding this comment.
Yes but I think instead of just merging this PR, we should actually discuss as a team how we should fix this and dont forget about this issue.
Thats why I was saying we should separate out regression test change from this PR and merge that and discuss how we want to handle contract upgrades because this might be an issue for the caff node
2 participants
Minor improvements: