Skip to content

Conversation

@nicholaspai
Copy link
Member

To aid with future usage of this adapter

@nicholaspai nicholaspai requested review from mrice32 and pxrl December 21, 2022 17:20
Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

Looks good! Only one minor nit (note: since this is already deployed, we can go ahead and use the existing implementation for this operation, but I think we should update the contract in the repo with the non-deprecated method).

bytes calldata data
) external payable returns (uint256);

function createRetryableTicketNoRefundAliasRewrite(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we use unsafeCreateRetryableTicket instead (I think the signature is the same)?

According to the current Inbox implementation on mainnet, createRetryableTicketNoRefundAliasRewrite is deprecated in favor of unsafeCreateRetryableTicket.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a great call. createRetryableTicketNoRefundAliasRewrite calls unsafeCreateRetryableTicket so I agree we don't need to change deployed version

@nicholaspai nicholaspai requested a review from mrice32 December 23, 2022 15:42
@nicholaspai
Copy link
Member Author

Copy link
Contributor

@pxrl pxrl left a comment

Choose a reason for hiding this comment

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

No comments from me 👍.

Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

LGTM!

@nicholaspai nicholaspai merged commit 9243374 into master Dec 27, 2022
@nicholaspai nicholaspai deleted the npai/arbitrum-rescue branch December 27, 2022 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants