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

feat: update depositTo to use standard deposit #432

Merged
merged 27 commits into from
Apr 30, 2024
Merged

Conversation

brtkx
Copy link
Contributor

@brtkx brtkx commented Mar 19, 2024

closes FS-368

@cla-bot cla-bot bot added the cla-signed label Mar 19, 2024
Copy link

vercel bot commented Mar 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
arbitrum-sdk ✅ Ready (Inspect) Visit Preview Apr 30, 2024 4:44pm

params.retryableGasOverrides
)

const tx = await inbox.connect(params.l1Signer).unsafeCreateRetryableTicket(
Copy link
Member

Choose a reason for hiding this comment

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

why are we using unsafeCreateRetryableTicket here? can be dangerous

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to createRetryableTicket

Copy link
Member

@gzeoneth gzeoneth left a comment

Choose a reason for hiding this comment

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

LGTM

'0x', // data,
{
from: signerAddress.value,
value: amount,
Copy link
Member

Choose a reason for hiding this comment

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

actually this should be

Suggested change
value: amount,
value: gasEstimation.deposit,

Copy link
Member

@gzeoneth gzeoneth left a comment

Choose a reason for hiding this comment

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

LGTM

destWallet.address
)
expect(
testWalletL2EthBalance.lt(ethToDeposit),
Copy link
Member

Choose a reason for hiding this comment

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

lets assert it's zero

@@ -211,6 +216,60 @@ describe('Ether', async () => {
)
})

it('deposit ether to a specific L2 address and check balance before auto-redeem', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('deposit ether to a specific L2 address and check balance before auto-redeem', async () => {
it('deposit ether to a specific L2 address with manual redeem', async () => {

const rec = await res.wait()
const l1ToL2Messages = await rec.getL1ToL2Messages(l2Signer.provider!)
expect(l1ToL2Messages.length).to.eq(1, 'failed to find 1 l1 to l2 message')
let l1ToL2Message = l1ToL2Messages[0]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let l1ToL2Message = l1ToL2Messages[0]
const l1ToL2MessageReader = l1ToL2Messages[0]

Comment on lines 260 to 265
l1ToL2Message = l1Receipt.getL1ToL2Messages(l2Signer)[0]

if (l1ToL2Message instanceof L1ToL2MessageWriter) {
const res = await l1ToL2Message.redeem()
await res.wait()
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
l1ToL2Message = l1Receipt.getL1ToL2Messages(l2Signer)[0]
if (l1ToL2Message instanceof L1ToL2MessageWriter) {
const res = await l1ToL2Message.redeem()
await res.wait()
}
const l1ToL2MessageWriter = l1Receipt.getL1ToL2Messages(l2Signer)[0]
await (await l1ToL2MessageWriter.redeem()).wait()

Copy link
Member

@spsjvc spsjvc left a comment

Choose a reason for hiding this comment

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

LGTM

@spsjvc spsjvc merged commit e277d33 into main Apr 30, 2024
24 checks passed
@spsjvc spsjvc deleted the depositTo-refactor branch April 30, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants