-
Notifications
You must be signed in to change notification settings - Fork 1
Chain cursing e2e test - Verifier side #314
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
base: main
Are you sure you want to change the base?
Conversation
| require.NoError(t, err) | ||
|
|
||
| l.Info().Msg("Asserting baseline message reaches verifier but gets dropped due to global curse") | ||
| testCtx.AssertMessageReachedAndDroppedInVerifier(messageID01, 100*time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a plan to also add test that shows how we recover those message once the curse is lifted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this replay features e2e tests should be part of M3
CC @makramkd
8782f5a to
1f90f75
Compare
|
Code coverage report:
|
E2E Smoke Test Results
Full logs are available in the workflow artifacts. |
| // ApplyCurse applies a curse to the specified subjects on the given chain. | ||
| ApplyCurse(ctx context.Context, chainSelector uint64, subjects [][16]byte) error | ||
|
|
||
| // ApplyUncurse removes a curse from the specified subjects on the given chain. | ||
| ApplyUncurse(ctx context.Context, chainSelector uint64, subjects [][16]byte) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can just be Curse and Uncurse as they are already verbs
| rmnRemoteAddr, err := m.getRMNRemoteAddress(chainSelector) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| ethClient, ok := m.ethClients[chainSelector] | ||
| if !ok { | ||
| return fmt.Errorf("eth client not found for chain %d", chainSelector) | ||
| } | ||
|
|
||
| rmnRemote, err := rmn_remote_binding.NewRMNRemote(rmnRemoteAddr, ethClient) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create RMN Remote contract binding: %w", err) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull this out into a private helper method getRMNRemote so that you can use from the Curse and Uncurse methods
| return fmt.Errorf("deployer key not found for chain %d", chainSelector) | ||
| } | ||
|
|
||
| // Set context for transaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless comment?
| Uint64("chain", chainSelector). | ||
| Str("tx", tx.Hash().Hex()). | ||
| Int("numSubjects", len(subjects)). | ||
| Msg("Applied curse on chain") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Msg("Applied curse on chain") | |
| Msg("Cursed subjects on chain") |
| m.logger.Info(). | ||
| Uint64("chain", chainSelector). | ||
| Str("tx", tx.Hash().Hex()). | ||
| Int("numSubjects", len(subjects)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The subjects are pertinent to this function, would be nice to see them (maybe as uint64's and not [16]byte)
| chain0, chain1, chain2 := selectors[0], selectors[1], selectors[2] | ||
| receiver := mustGetEOAReceiverAddress(t, c, chain1) | ||
|
|
||
| sentEvt := testCtx.MustSendMessage(chain0, chain1, receiver, 150) // Use custom finality to slow down picking for verification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the makings of a flakey test because if we somehow speed up the mining even more this test may start randomly failing?
More effective would be to use an env.toml file that doesn't have super fast finality?
| // We sleep here because in reality we'll need to replay events in case of curses to pick up the dropped tasks | ||
| // Increased wait time for CI environments where services may need more time to catch up | ||
| time.Sleep(15 * time.Second) | ||
|
|
||
| testCtx.MustExecuteMessage(chain0, chain1, receiver, 0) // finality=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what happens to sentEvt, this first message sent in this test?
| l.Info().Msg("Asserting baseline message reaches verifier but gets dropped due to curse") | ||
| testCtx.AssertMessageReachedAndDroppedInVerifier(messageID, 100*time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like we need to come up with a better way to assert things like this more than ever, since this is a pretty long timeout (longer than half the other tests in TestSmoke combined)
| // DefaultMessageFields creates a simple message with test data. | ||
| func DefaultMessageFields(receiver protocol.UnknownAddress) cciptestinterfaces.MessageFields { | ||
| return cciptestinterfaces.MessageFields{ | ||
| Receiver: receiver, | ||
| Data: []byte("test-msg"), | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't need to be exported
| // MustSendMessage sends a message and waits for the sent event. | ||
| // If finality is 0, uses version 2 (no finality config). | ||
| // If finality is non-zero, uses version 3 with the specified finality config. | ||
| func (tc *TestingContext) MustSendMessage(srcChain, destChain uint64, receiver protocol.UnknownAddress, finality uint16) cciptestinterfaces.MessageSentEvent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really like to not have these helpers - feels like this could be part of the top-level interface instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can define DefaultMessageFields as a test-local thing instead of something thats hardcoded in the method.
No description provided.