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(sequencer)!: use bridge address to determine asset in bridge unlock cost estimation instead of signer #1905

Merged
merged 5 commits into from
Jan 13, 2025

Conversation

ethanoroshiba
Copy link
Contributor

@ethanoroshiba ethanoroshiba commented Jan 9, 2025

Summary

Changed bridge unlock cost calculation to use bridge address instead of signer address.

Background

Previously, the get_total_transaction_cost attempted to retrieve the bridge account asset by the tx signer instead of by the bridge address. This works fine if the sender is a bridge account, but if it is the authorized bridge withdrawer, it will fail.

Changes

  • Change BridgeUnlock cost calculation to retrieve asset based on the action's bridge address instead of the transaction signer.

Testing

Added unit test to ensure function works correctly when submitting a BridgeUnlock from the bridge withdrawer address.

Synced to mainnet from genesis to ensure this would not be a network breaking change.

Changelogs

Changelog updates.

Breaking Changelist

  • This change is breaking since previous BridgeUnlock submitted from the withdrawer address would fail. This has been synced to mainnet successfully from genesis as of January 13, 2025 at 2:32pm CST.

Related Issues

closes #1904

@ethanoroshiba ethanoroshiba added bug Something isn't working sequencer pertaining to the astria-sequencer crate labels Jan 9, 2025
@ethanoroshiba ethanoroshiba marked this pull request as ready for review January 9, 2025 19:25
@ethanoroshiba ethanoroshiba requested a review from a team as a code owner January 9, 2025 19:25
@ethanoroshiba ethanoroshiba changed the title fix(sequencer)!: fix bridge unlock cost calculation fix(sequencer)!: use bridge address to determine asset in bridge unlock cost estimation instead of signer Jan 10, 2025
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

This reads like the right change to make, but the naming of the containing function, get_total_transaction_cost, is highly confusing to me. This likely played a role in this bug being introduced in the first place.

I remember coming across this function earlier and being caught by this.

I think we should remove this function entirely in favor of two functions, calculate_fees_for_transaction (already exists) and calculate_funds_moved_by_transaction (essentially move the loop over the actions to it). The check for enough funds would then take the result of both methods.

I think that would make the current situation much clearer.

@ethanoroshiba ethanoroshiba added this pull request to the merge queue Jan 13, 2025
Merged via the queue into main with commit 5c4feaf Jan 13, 2025
49 checks passed
@ethanoroshiba ethanoroshiba deleted the eoroshiba/fix_bridge_unlock_cost_calculation branch January 13, 2025 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(sequencer): fix bridge unlock cost calculation
3 participants