Backport shim fix to eth-bridge-integration #689
Conversation
| let (processing_results, _) = | ||
| self.service.process_proposed_txs(&self.delivered_txs); |
There was a problem hiding this comment.
This is the only sort of irrelevant line that should differ between eth-bridge-integration and v0.8.1 now - in eth-bridge-integration we need to return a DigestCounters as part of process_txs/process_proposed_txs
| /// present. `ProcessProposal` should be able to make a decision on whether | ||
| /// a proposed block is acceptable or not based solely on what this function | ||
| /// returns. | ||
| pub fn process_proposed_txs( |
There was a problem hiding this comment.
Suggest we take the opportunity to rename this fn (and process_single_tx), since we are changing the signatures of these two fns anyway
|
pls update wasm |
| ) | ||
| }) | ||
| .collect(); | ||
| let (tx_results, counters) = self.process_proposed_txs(&req.txs); | ||
|
|
||
| // We should not have more than one `ethereum_events::VextDigest` in | ||
| // a proposal from some round's leader. |
There was a problem hiding this comment.
might as well move the logic from lines 55-110 to a new function. something like:
fn vote_on_proposal(tx_results: &[TxResult], counters: &DigestCounters) -> ProposalStatusThere was a problem hiding this comment.
agree though it ends being a bit more of a refactor so would rather not do it in this PR. Specifically we currently log the reason for rejecting a proposed block, with context from the req struct, which we wouldn't be able to do in a fn vote_on_proposal without also passing in req or returning the reason (e.g. via an enum), I think we should go with something like the latter
| /// present. `ProcessProposal` should be able to make a decision on whether | ||
| /// a proposed block is acceptable or not based solely on what this function | ||
| /// returns. | ||
| pub fn process_proposed_txs( |
There was a problem hiding this comment.
I'd rename this to something like check_tx_status(), or anything that conveys the fact all we are doing is checking if a given tx is valid.
There was a problem hiding this comment.
Agreed, technically we could "process" txs during ProcessProposal but in our ABCI application we don't do this which keeps things simpler. ProcessProposal to us is really more like CheckProposal
There was a problem hiding this comment.
renamed to check_proposal and check_proposal_tx
| /// Calculates what the [`TxResult`]s would be for the transactions in a | ||
| /// proposed block, as well as counting the number of digest transactions | ||
| /// present. `ProcessProposal` should be able to make a decision on whether | ||
| /// a proposed block is acceptable or not based solely on what this function | ||
| /// returns. |
There was a problem hiding this comment.
| /// Calculates what the [`TxResult`]s would be for the transactions in a | |
| /// proposed block, as well as counting the number of digest transactions | |
| /// present. `ProcessProposal` should be able to make a decision on whether | |
| /// a proposed block is acceptable or not based solely on what this function | |
| /// returns. | |
| /// Calculates what the [`TxResult`]s would be for the transactions in a | |
| /// proposed block, and counts the number of digest transactions | |
| /// present. |
leave the last part to vote_on_proposal, with a bit of clean up:
/// Decides whether `ProcessProposal` should accept or reject
/// a block proposed by some Tendermint round's leader.
///
/// We first call `check_tx_status()`, to check for non-recoverable
/// transaction errors.
There was a problem hiding this comment.
unsure about this suggestion, if we don't create a vote_on_proposal fn in this PR
Fixes #641 for
eth-bridge-integrationThis is a backport of #656 which is in namada v0.8.1 (backporting now so that merging v0.8.1 into
eth-bridge-integrationis easier -Relates to #673)
We had removed
Shell::process_txsineth-bridge-integration, which is now being used by the shim, so had to add something like it back -Shell::process_proposed_txs.