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

refactor stateless checks to be done on types with withdrawals #1430

Closed
joroshiba opened this issue Aug 29, 2024 · 0 comments · Fixed by #1391 · May be fixed by #1770
Closed

refactor stateless checks to be done on types with withdrawals #1430

joroshiba opened this issue Aug 29, 2024 · 0 comments · Fixed by #1391 · May be fixed by #1770

Comments

@joroshiba
Copy link
Member

joroshiba commented Aug 29, 2024

PR #1391 introduces some stateless checks in BridgeUnlockAction and Ics20WithdrawalAction which are very similar. This is done because of parsing done in stateless check of memo for Ics20WithdrawalAction.

As a follow up we should update the canonical domain rust types to doing this validation during parsing, ideally there is some shared logic so it can be kept insync between the two actions.

┆Issue Number: ENG-766

github-merge-queue bot pushed a commit that referenced this issue Aug 29, 2024
)

## Summary
Adds state for bridge events and consumes the events as they occur.
Updates proto for bridge unlock to no longer use memo for rollup
information and instead include on main action. Renames
`rollup_transaction_hash` to `rollup_withdrawal_event_id`.

## Background
There was only implicit stop against reusing a withdrawal event, this
adds consumption of withdrawal events. While this is not strictly
security enhancing, consuming the event adds in protocol protection
against accidental double spend by the bridge operator.

## Changes
- Proto updates for `BridgeUnlockAction` + `ICS20WithdrawalMemo`
- Add stateless checks on bridging unlock and withdrawals of correct
information filled out
- Added state to enforce rollup event cannot be consumed twice. 
- Enforce that `Ics20WithdrawalAction` must have `bridge_address` set if
making a bridge withdrawal

## Testing
smoke test + minor test updates

## Breaking Changelist
- New stateful information about rollup withdrawal events
- Adds new fields to `BridgeUnlockAction`

## Related Issues
closes #1430

---------

Co-authored-by: Fraser Hutchison <[email protected]>
Co-authored-by: Richard Janis Goldschmidt <[email protected]>
jbowen93 pushed a commit that referenced this issue Sep 3, 2024
)

## Summary
Adds state for bridge events and consumes the events as they occur.
Updates proto for bridge unlock to no longer use memo for rollup
information and instead include on main action. Renames
`rollup_transaction_hash` to `rollup_withdrawal_event_id`.

## Background
There was only implicit stop against reusing a withdrawal event, this
adds consumption of withdrawal events. While this is not strictly
security enhancing, consuming the event adds in protocol protection
against accidental double spend by the bridge operator.

## Changes
- Proto updates for `BridgeUnlockAction` + `ICS20WithdrawalMemo`
- Add stateless checks on bridging unlock and withdrawals of correct
information filled out
- Added state to enforce rollup event cannot be consumed twice. 
- Enforce that `Ics20WithdrawalAction` must have `bridge_address` set if
making a bridge withdrawal

## Testing
smoke test + minor test updates

## Breaking Changelist
- New stateful information about rollup withdrawal events
- Adds new fields to `BridgeUnlockAction`

## Related Issues
closes #1430

---------

Co-authored-by: Fraser Hutchison <[email protected]>
Co-authored-by: Richard Janis Goldschmidt <[email protected]>
jbowen93 pushed a commit that referenced this issue Sep 6, 2024
)

## Summary
Adds state for bridge events and consumes the events as they occur.
Updates proto for bridge unlock to no longer use memo for rollup
information and instead include on main action. Renames
`rollup_transaction_hash` to `rollup_withdrawal_event_id`.

## Background
There was only implicit stop against reusing a withdrawal event, this
adds consumption of withdrawal events. While this is not strictly
security enhancing, consuming the event adds in protocol protection
against accidental double spend by the bridge operator.

## Changes
- Proto updates for `BridgeUnlockAction` + `ICS20WithdrawalMemo`
- Add stateless checks on bridging unlock and withdrawals of correct
information filled out
- Added state to enforce rollup event cannot be consumed twice. 
- Enforce that `Ics20WithdrawalAction` must have `bridge_address` set if
making a bridge withdrawal

## Testing
smoke test + minor test updates

## Breaking Changelist
- New stateful information about rollup withdrawal events
- Adds new fields to `BridgeUnlockAction`

## Related Issues
closes #1430

---------

Co-authored-by: Fraser Hutchison <[email protected]>
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
Labels
None yet
Projects
None yet
1 participant