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.

Sign up for GitHub

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

Jump to bottom

Restrictions on bridge-unlock-action contents are too strict #1471

Closed
SuperFluffy opened this issue Sep 9, 2024 · 0 comments
Closed

Restrictions on bridge-unlock-action contents are too strict #1471

SuperFluffy opened this issue Sep 9, 2024 · 0 comments
Assignees
@itamarreif

Comments

@SuperFluffy
Copy link
Member

SuperFluffy commented Sep 9, 2024 •
edited by sync-by-unito bot
Loading

Sequencer currently imposes that the field astria.protocol.transactions.v1alpha1.BridgeUnlockAction.rollup_withdrawal_event_id has a length no more than 64 bytes.

This creates an issue when wanting to write the native formatting of a eth TX hash into this field, which is of the form 0x<hex-of-64-bytes>, for a total length of 130 bytes.

Very likely the restriction on the memo length is similarly too strict.

astria/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs

Lines 28 to 46 in fc9f225

impl ActionHandler for BridgeUnlockAction {
// TODO(https://github.com/astriaorg/astria/issues/1430): move checks to the `BridgeUnlock` parsing.
async fn check_stateless(&self) -> Result<()> {
ensure!(self.amount > 0, "amount must be greater than zero",);
ensure!(self.memo.len() <= 64, "memo must not be more than 64 bytes");
ensure!(
!self.rollup_withdrawal_event_id.is_empty(),
"rollup withdrawal event id must be non-empty",
);
ensure!(
self.rollup_withdrawal_event_id.len() <= 64,
"rollup withdrawal event id must not be more than 64 bytes",
);
ensure!(
self.rollup_block_number > 0,
"rollup block number must be greater than zero",
);
Ok(())
}

┆Issue Number: ENG-802
@itamarreif itamarreif self-assigned this Sep 13, 2024
@itamarreif itamarreif mentioned this issue Sep 13, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Sep 16, 2024
@itamarreif @SuperFluffy
fix(bridge-contracts): fix memo transaction hash encoding (#1428)
6b5dae9 
## Summary
Use the correct encoding functions to populate memo field in the
bridge's conversion logic from rollup withdrawal events to sequencer
actions.

## Background
We were using `ethers::H256::to_string()` to get the string for the
`rollup_transaction_hash` value, which returns a shortened version of
the hash. the string `0x1234...0x1234` is instead of the full hash (i.e.
you would expect it to give you something like
`0x1234567890123456789012345678901234567890123456789012345678901234`).

## Changes
- Use the correct 

## Breaking Changelist
- The previously invalid memo data will now be populated with the
correct information. This doesn't actually break anything because we
also aren't actually using these hashes anywhere.

## Related Issues
Link any issues that are related, prefer full github links.

closes #1427, #1471

---------

Co-authored-by: Richard Janis Goldschmidt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@itamarreif itamarreif

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@SuperFluffy @itamarreif