-
Notifications
You must be signed in to change notification settings - Fork 11.7k
Bridge limiter bypass #24221
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
base: main
Are you sure you want to change the base?
Bridge limiter bypass #24221
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| target_address: vector<u8>, | ||
| token_type: u8, | ||
| amount: u64, | ||
| timestamp: u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timestamp_ms
|
|
||
| // check system ops seq number and increment it | ||
| let expected_seq_num = inner.get_current_seq_num_and_increment(message_type); | ||
| let expected_seq_num = inner.get_current_seq_num_and_increment( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats with the inconsistent formatting or rather reformatting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this formatting is using "npx prettier-move -w **/*.move". I think Damir formatted this package a while back. let me touch base with him
e243cac to
2e01732
Compare
crates/sui-bridge/src/abi.rs
Outdated
| #[serde(default)] | ||
| pub block_timestamp: Option<u64>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you just add things here? i don't know enough about the serialization format for eth messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no this is a mistake. We should not be updating the event emitted
| pub sui_bridge_event: EmittedSuiToEthTokenBridgeV1, | ||
| } | ||
|
|
||
| impl SuiToEthBridgeAction { | ||
| pub fn timestamp_seconds(&self) -> Option<u64> { | ||
| self.sui_bridge_event.timestamp_ms.map(|ms| ms / 1000) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this may not work, and we'll likely want a full SuiToEthBridgeActionV2 type since the timestamp can't just be added as its a breaking change to the serialized format that we store in the db :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
| public fun extract_token_bridge_payload_v2_timestamp(message: &BridgeMessage): u64 { | ||
| let mut bcs = bcs::new(message.payload); | ||
| let _sender_address = bcs.peel_vec_u8(); | ||
| let _target_chain = bcs.peel_u8(); | ||
| let _target_address = bcs.peel_vec_u8(); | ||
| let _token_type = bcs.peel_u8(); | ||
| let _amount = peel_u64_be(&mut bcs); | ||
| let timestamp = peel_u64_be(&mut bcs); | ||
|
|
||
| assert!(bcs.into_remainder_bytes().is_empty(), ETrailingBytes); | ||
|
|
||
| timestamp | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably would be better to deser into the payload v2 struct itself and then have a function to get out the timestamp on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a result of trying to avoid introducing a "claim_token_v2" so that all messages regardless of version can be used in the same function.
| chain_ids::assert_valid_chain_id(target_chain); | ||
| assert!(bcs.into_remainder_bytes().is_empty(), ETrailingBytes); | ||
|
|
||
| if (message.message_version() == 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably add a v2 payload type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will rework it
530d262 to
238d5bb
Compare
Description
Describe the changes or additions included in this PR.
Test plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.