fix(sequencer)!: use bridge address to determine asset in bridge unlock cost estimation instead of signer #1905
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SuperFluffy
reviewed
Jan 10, 2025
SuperFluffy
approved these changes
Jan 10, 2025
Contributor
SuperFluffy
left a comment
There was a problem hiding this comment.
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.
This was referenced Jan 10, 2025
noot
approved these changes
Jan 13, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 21, 2025
## Summary Refactored calculation of total transfers made in a transaction to helper function for clarity. ## Background See here: #1905 (review) > 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. ## Changes - Moved calculation of all transfers made in a transaction to helper function, and called this helper function in `get_total_transaction_cost()`. ## Testing Passing all tests, no additional tests needed. ## Changelogs No updates required. ## Related Issues closes #1907
sgranfield4403-3
added a commit
to sgranfield4403-3/astria
that referenced
this pull request
Oct 2, 2025
## Summary Refactored calculation of total transfers made in a transaction to helper function for clarity. ## Background See here: astriaorg/astria#1905 (review) > 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. ## Changes - Moved calculation of all transfers made in a transaction to helper function, and called this helper function in `get_total_transaction_cost()`. ## Testing Passing all tests, no additional tests needed. ## Changelogs No updates required. ## Related Issues closes #1907
AngieD101
added a commit
to AngieD101/astria
that referenced
this pull request
Oct 10, 2025
## Summary Refactored calculation of total transfers made in a transaction to helper function for clarity. ## Background See here: astriaorg/astria#1905 (review) > 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. ## Changes - Moved calculation of all transfers made in a transaction to helper function, and called this helper function in `get_total_transaction_cost()`. ## Testing Passing all tests, no additional tests needed. ## Changelogs No updates required. ## Related Issues closes #1907
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Changed bridge unlock cost calculation to use bridge address instead of signer address.
Background
Previously, the
get_total_transaction_costattempted 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
BridgeUnlockcost 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
BridgeUnlockfrom the bridge withdrawer address.Synced to mainnet from genesis to ensure this would not be a network breaking change.
Changelogs
Changelog updates.
Breaking Changelist
BridgeUnlocksubmitted 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