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

fix(bridge-history): incorrect status update and duplicated failed txs #1088

Merged
merged 15 commits into from
Jan 23, 2024

Conversation

colinlyguo
Copy link
Member

@colinlyguo colinlyguo commented Jan 22, 2024

Purpose or design rationale of this PR

This PR fixes several bugs:

  1. failed relayed tx is not a terminal status, _to.call can fail with OOG and emit FailedRelayedMessage, in this case, retry will help.
  2. remove db transaction because (i) all updates are idempotent. (ii) if during a transaction, the status is updated as "relayed" or "drop", the status would be incorrectly reset to "sent" in replayMessage scenario.
  3. tweak InsertFailedGatewayRouterTxs to avoid inserting same transaction multiple times, which is very unfriendly to pagination.

Bridge cost optimization incompatibility:
5. Fix 1: EOA -> gateway router case.
6. Fix 2: EOA -> multisig -> gateway router case.

if tx.To() != nil && (*tx.To()).String() != e.cfg.GatewayRouterAddr { // EOA -> multisig -> gateway router.
	from = (*tx.To()).String()
} else { // EOA -> gateway router.
	signer := types.LatestSignerForChainID(new(big.Int).SetUint64(tx.ChainId().Uint64()))
	sender, senderErr := signer.Sender(tx)
	if senderErr != nil {
		log.Error("get sender failed", "chain id", tx.ChainId().Uint64(), "tx hash", tx.Hash().String(), "err", senderErr)
		return nil, nil, senderErr
	}
	from = sender.String()
}

PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • fix: A bug fix

Deployment tag versioning

Has tag in common/version.go been updated or have you added bump-version label to this PR?

  • Yes

Breaking change label

Does this PR have the breaking-change label?

  • Yes

@colinlyguo colinlyguo added bug Something isn't working breaking-change labels Jan 22, 2024
@colinlyguo colinlyguo self-assigned this Jan 22, 2024
Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c68f428) 56.49% compared to head (07426cf) 56.49%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1088      +/-   ##
===========================================
- Coverage    56.49%   56.49%   -0.01%     
===========================================
  Files          149      149              
  Lines        11114    11115       +1     
===========================================
  Hits          6279     6279              
- Misses        4406     4407       +1     
  Partials       429      429              
Flag Coverage Δ
bridge-history-api 72.50% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@colinlyguo colinlyguo added the bump-version Bump the version tag for deployment label Jan 22, 2024
georgehao
georgehao previously approved these changes Jan 22, 2024
@colinlyguo colinlyguo force-pushed the fix-bridge-history-backend branch from d7da13f to 7bb2c34 Compare January 22, 2024 17:28
@georgehao georgehao merged commit c6e8fcb into develop Jan 23, 2024
12 checks passed
@georgehao georgehao deleted the fix-bridge-history-backend branch January 23, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change bug Something isn't working bump-version Bump the version tag for deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants