Skip to content

fix: surface eth deposits in migration package#87

Closed
fredlacs wants to merge 3 commits intoc-nitro-migrationfrom
fix-eth-deposits
Closed

fix: surface eth deposits in migration package#87
fredlacs wants to merge 3 commits intoc-nitro-migrationfrom
fix-eth-deposits

Conversation

@fredlacs
Copy link
Contributor

@fredlacs fredlacs commented Jun 16, 2022

When using sdk v1 users would get the eth deposit message when interacting with the migration sdk
With the nitro sdk v3, you need a separate function call to get eth deposits.

We should bump up the nitro package to include #86 so the status function is also available

We could attempt to make EthDepositMessage closer to a IL1ToL2MessageReaderOrWriter but most methods would throw errors since they don't make sense.

Instead making a separate entrypoint function getAllDeposits, need to publish new nitro beta version to consume this here though

@fredlacs fredlacs requested a review from yahgwai June 16, 2022 14:29
@cla-bot cla-bot bot added the cla-signed label Jun 16, 2022
this.nitroReceipt = new nitro.L1TransactionReceipt(tx)
}

public async getAllDeposits<T extends SignerOrProvider>(l2SignerOrProvider: T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think we should add a getAllDeposits func, instead we should a getEthDepositMessages func like we have in nitro, then use the toNitroEthDepositMessage function to convert the underlying messages into a nitro format

@fredlacs fredlacs changed the title fix: include eth deposits in migration's L1 to L2 message fix: surface eth deposits in migration package Jun 20, 2022
@fredlacs
Copy link
Contributor Author

closed in favour of #88

@fredlacs fredlacs closed this Jun 20, 2022
@fredlacs fredlacs deleted the fix-eth-deposits branch June 20, 2022 13:10
umershaikh123 pushed a commit to umershaikh123/arbitrum-sdk that referenced this pull request May 20, 2024
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.

2 participants