[CORE-709] do not error when completing a bridge with non-positive amount #691

tqin7 · 2023-10-24T17:51:59Z

Changelist

When completing a bridge with non-positive amount, do not error. Simply ignore bank transfer and consider the bridge completed.

Approaches considered for bridges with 0 amount

Still complete it but skip bank transfer (what this PR implements)

pros
- all bridges follow the same logic path in bridge module, leading to more consistent behaviors and metrics (for example, we emit metric on completed bridge ID in CompleteBridge)
- state-compatible (delaymsg module state is the same as before)
cons:
- does not save storage (but I think this is fine as any bridge, as long as acknowledged by the chain, "deserves" to have a MsgCompleteBridge scheduled for it (as gas has been paid anyways on Ethereum side)

When acknowledging the bridge, do not schedule a delayed MsgCompleteBridge

pros
- saves storage
cons:
- not state-compatible (delaymsg module state is not the same as before)

Test Plan

[Describe how this PR was tested (if applicable)]

Author/Reviewer Checklist

If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
Manually add any of the following labels: refactor, chore, bug.

linear · 2023-10-24T17:52:04Z

CORE-709 handle bridge event with amount = 0

coderabbitai · 2023-10-24T17:52:05Z

Walkthrough

The changes primarily focus on enhancing the logic and accuracy of the complete_bridge.go and its corresponding test file complete_bridge_test.go. The updates introduce a conditional check to ensure only positive coin amounts are sent from the bridge module account. The test function TestCompleteBridge is also refined to reflect these changes and improve clarity.

Changes

File	Summary
`.../keeper/complete_bridge.go`	Introduced a conditional check to ensure only positive coin amounts are sent from the bridge module account.
`.../keeper/complete_bridge_test.go`	Renamed variables for clarity and updated test cases to check for zero and negative coin amounts. Adjusted expected balance assertions to reflect these changes.

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

If you reply to a review comment from CodeRabbit, the bot will automatically respond.
To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
Note: Review comments are made on code diffs or files, not on the PR overview.
Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between a612627 and 5eca091.

Files selected for processing (2)

protocol/x/bridge/keeper/complete_bridge.go (1 hunks)
protocol/x/bridge/keeper/complete_bridge_test.go (5 hunks)

Files skipped from review due to trivial changes (2)

protocol/x/bridge/keeper/complete_bridge.go
protocol/x/bridge/keeper/complete_bridge_test.go

teddyding · 2023-10-24T20:31:15Z

protocol/x/bridge/keeper/complete_bridge.go

+	// If coin amount is positive, send coin from bridge module account to
+	// specified account.
+	if bridge.Coin.Amount.IsPositive() {
+		err = k.bankKeeper.SendCoinsFromModuleToAccount(


nit:

if err = .....; err != nil { return err }

teddyding · 2023-10-24T20:33:25Z

protocol/x/bridge/keeper/complete_bridge.go

-		return err
+	// If coin amount is positive, send coin from bridge module account to
+	// specified account.
+	if bridge.Coin.Amount.IsPositive() {


I see we have taken the approach to still schedule delayed messages for 0 transfers.
Can you document somewhere the rationale behind tradeoffs of different approaches, and why we are allowing zero-transfer delay messages to be added to state?

Documented rationale in PR description!

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 5eca091 and 859030f.

Files selected for processing (2)

protocol/x/bridge/keeper/complete_bridge.go (1 hunks)
protocol/x/bridge/keeper/complete_bridge_test.go (5 hunks)

Files skipped from review due to trivial changes (2)

protocol/x/bridge/keeper/complete_bridge.go
protocol/x/bridge/keeper/complete_bridge_test.go

…ount (#691) * do not error when completing a bridge with non-positive amount (cherry picked from commit 525873d) # Conflicts: # protocol/x/bridge/keeper/complete_bridge_test.go

…ount (backport #691) (#707) * [CORE-709] do not error when completing a bridge with non-positive amount (#691) * do not error when completing a bridge with non-positive amount (cherry picked from commit 525873d) # Conflicts: # protocol/x/bridge/keeper/complete_bridge_test.go * fix merge conflict --------- Co-authored-by: Tian <tian@dydx.exchange>

do not error when completing a bridge with non-positive amount

5eca091

github-actions bot added the protocol label Oct 24, 2023

coderabbitai bot reviewed Oct 24, 2023

View reviewed changes

clemire approved these changes Oct 24, 2023

View reviewed changes

teddyding reviewed Oct 24, 2023

View reviewed changes

tqin7 added 2 commits October 25, 2023 13:14

Merge branch 'main' into tq/core-709

b708416

address nit

859030f

coderabbitai bot reviewed Oct 25, 2023

View reviewed changes

teddyding approved these changes Oct 25, 2023

View reviewed changes

tqin7 added the backport/protocol/v1.x Backport v1.x label Oct 25, 2023

tqin7 merged commit 525873d into main Oct 25, 2023
15 of 16 checks passed

tqin7 deleted the tq/core-709 branch October 25, 2023 23:15

mergify bot mentioned this pull request Oct 25, 2023

[CORE-709] do not error when completing a bridge with non-positive amount (backport #691) #707

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CORE-709] do not error when completing a bridge with non-positive amount #691

[CORE-709] do not error when completing a bridge with non-positive amount #691

tqin7 commented Oct 24, 2023 •

edited

Loading

linear bot commented Oct 24, 2023

coderabbitai bot commented Oct 24, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

coderabbitai bot left a comment

teddyding Oct 24, 2023 •

edited

Loading

teddyding Oct 24, 2023

tqin7 Oct 25, 2023

coderabbitai bot left a comment

[CORE-709] do not error when completing a bridge with non-positive amount #691

[CORE-709] do not error when completing a bridge with non-positive amount #691

Conversation

tqin7 commented Oct 24, 2023 • edited Loading

Changelist

Test Plan

Author/Reviewer Checklist

linear bot commented Oct 24, 2023

coderabbitai bot commented Oct 24, 2023 • edited Loading

Walkthrough

Changes

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

coderabbitai bot left a comment

Choose a reason for hiding this comment

teddyding Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

teddyding Oct 24, 2023

Choose a reason for hiding this comment

tqin7 Oct 25, 2023

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

tqin7 commented Oct 24, 2023 •

edited

Loading

coderabbitai bot commented Oct 24, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)

teddyding Oct 24, 2023 •

edited

Loading