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

Integration test for balances and (non)retryable messages #2505

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rafal-ch
Copy link
Contributor

@rafal-ch rafal-ch commented Dec 17, 2024

Closes #2474

Description

This PR changes the following:

  1. Adds new test in tests/tests/relayer.rs - to ensure that only non-retryable messages contribute to the balance returned from balance(), balances() and coins_to_spend()1 endpoints
  2. Modifies the balance() test in tests/tests/balances.rs to also include some retryable message which is expected not to contribute to the total balance.
    • for balances() and coins_to_spend() it was already the case

1st test checks the behavior of the actual blockchain operations while the 2nd tests the proper initialization of the balances index at genesis.

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests

Before requesting review

  • I have reviewed the code myself

Footnotes

  1. currently, coins_to_spend() is not using indexation, this will change when this PR is merged.

// Balances are processed in the off-chain worker, so we need to wait for it
// to process the messages before we can assert the balances.
let result = tokio::time::timeout(TIMEOUT, async {
loop {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better way to ensure that off chain worker has processed the events?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. Ideally we should be able to register and listen for events from the tracing module.
To me it looks like waiting for the balances to be available is a viable alternative

@rafal-ch rafal-ch added the no changelog Skip the CI check of the changelog modification label Dec 17, 2024
@rafal-ch rafal-ch marked this pull request as ready for review December 17, 2024 12:43
@rafal-ch rafal-ch requested a review from a team December 17, 2024 12:43
Copy link
Contributor

@acerone85 acerone85 left a comment

Choose a reason for hiding this comment

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

Thanks. I could follow the code without problems (comments helped), and checked that tests pass.

Comment on lines +512 to +521
let messages = vec![
MessageKind::Retryable {
nonce: RETRYABLE_NONCE,
amount: RETRYABLE_AMOUNT,
},
MessageKind::NonRetryable {
nonce: NON_RETRYABLE_NONCE,
amount: NON_RETRYABLE_AMOUNT,
},
];
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity, what's expected to happen if you assign the same nonce to both messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If recipient is also the same one message will overwritten by the other in DB. See here.

A lack of explicit checks suggest that we assume that such message will never come from the relayer.

IIUC, we don't have any special handling if two or more messages have the same nonce but different recipients.

eth_node.update_data(|data| data.logs_batch = vec![logs.clone()]);
// Setup the eth node with a block high enough that there
// will be some finalized blocks.
eth_node.update_data(|data| data.best_block.number = Some(200.into()));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do we have a constant defined somewhere for the L1 finalization period?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None that I know of.

// Balances are processed in the off-chain worker, so we need to wait for it
// to process the messages before we can assert the balances.
let result = tokio::time::timeout(TIMEOUT, async {
loop {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. Ideally we should be able to register and listen for events from the tracing module.
To me it looks like waiting for the balances to be available is a viable alternative

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Skip the CI check of the changelog modification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add integration tests for balances with (non)retryable messages
2 participants