-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
base: master
Are you sure you want to change the base?
Integration test for balances and (non)retryable messages #2505
Conversation
// 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 { |
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 better way to ensure that off chain worker has processed the events?
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 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
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.
Thanks. I could follow the code without problems (comments helped), and checked that tests pass.
let messages = vec![ | ||
MessageKind::Retryable { | ||
nonce: RETRYABLE_NONCE, | ||
amount: RETRYABLE_AMOUNT, | ||
}, | ||
MessageKind::NonRetryable { | ||
nonce: NON_RETRYABLE_NONCE, | ||
amount: NON_RETRYABLE_AMOUNT, | ||
}, | ||
]; |
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.
out of curiosity, what's expected to happen if you assign the same nonce to both messages?
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.
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())); |
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: do we have a constant defined somewhere for the L1 finalization period?
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.
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 { |
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 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
Closes #2474
Description
This PR changes the following:
tests/tests/relayer.rs
- to ensure that only non-retryable messages contribute to the balance returned frombalance()
,balances()
andcoins_to_spend()
1 endpointsbalance()
test intests/tests/balances.rs
to also include some retryable message which is expected not to contribute to the total balance.balances()
andcoins_to_spend()
it was already the case1st test checks the behavior of the actual blockchain operations while the 2nd tests the proper initialization of the balances index at genesis.
Checklist
Before requesting review
Footnotes
currently,
coins_to_spend()
is not using indexation, this will change when this PR is merged. ↩