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

Unlock DAO failed with light client #316

Closed
yanguoyu opened this issue Nov 12, 2023 · 8 comments · Fixed by nervosnetwork/neuron#2945
Closed

Unlock DAO failed with light client #316

yanguoyu opened this issue Nov 12, 2023 · 8 comments · Fixed by nervosnetwork/neuron#2945
Assignees
Labels
bug Something isn't working

Comments

@yanguoyu
Copy link

RPC method calculateDaoMaximumWithdraw was not in the light client. After replacing ckb-sdk-js with lumos, it throws an error that method not found. The calculateDaoMaximumWithdraw method in ckb-sdk-js is not the same as the lumos.

calculateDaoMaximumWithdraw in ckb-sdk-js
https://github.com/ckb-js/ckb-sdk-js/blob/develop/packages/ckb-sdk-core/src/index.ts#L423-L452
calculateDaoMaximumWithdraw in lumos
https://github.com/ckb-js/lumos/blob/develop/packages/rpc/src/Base/experimental.ts#L10-L13

@Keith-CY
Copy link
Member

Keith-CY commented Nov 12, 2023

@Danie0918 Danie0918 added the bug Something isn't working label Nov 13, 2023
@Danie0918 Danie0918 assigned yanguoyu and homura and unassigned yanguoyu Nov 13, 2023
@homura
Copy link

homura commented Nov 13, 2023

Maybe we can migrate to this method calculateMaximumWithdrawCompatible, but it is a little more work than before.

public calculateDaoMaximumWithdraw = async (
    depositOutPoint: OutPoint,
    withdrawBlockHash: string
  ): Promise<bigint> => {
    const currentNetwork = NetworksService.getInstance().getCurrent()
    const ckb = new CKBRPC(currentNetwork.remote)
-   const result = await ckb.calculateDaoMaximumWithdraw(depositOutPoint.toSDK(), withdrawBlockHash)
+   const depositHeader = await ckb.getHeader(depositBlockHash)
+   const withdrawHeader = await ckb.getHeader(withdrawBlockHash)
+   const withdrawCell = await ckb.getLiveCell(depositOutPoint.txHash)
+   return calculateMaximumWithdrawCompatible(withdrawCell, depositHeader.dao, withdrawHeader.dao)
  }

@Keith-CY
Copy link
Member

Keith-CY commented Nov 13, 2023

Maybe we can migrate to this method calculateMaximumWithdrawCompatible, but it is a little more work than before.

public calculateDaoMaximumWithdraw = async (
    depositOutPoint: OutPoint,
    withdrawBlockHash: string
  ): Promise<bigint> => {
    const currentNetwork = NetworksService.getInstance().getCurrent()
    const ckb = new CKBRPC(currentNetwork.remote)
-   const result = await ckb.calculateDaoMaximumWithdraw(depositOutPoint.toSDK(), withdrawBlockHash)
+   const depositHeader = await ckb.getHeader(depositBlockHash)
+   const withdrawHeader = await ckb.getHeader(withdrawBlockHash)
+   const withdrawCell = await ckb.getLiveCell(depositOutPoint.txHash)
+   return calculateMaximumWithdrawCompatible(withdrawCell, depositHeader.dao, withdrawHeader.dao)
  }

I found the method calculateDaoMaximumWithdraw can be done with this feature implementation Magickbase/ckb-explorer-public-issues#462 in CKB Explorer.

@yanguoyu
Copy link
Author

yanguoyu commented Nov 16, 2023

const withdrawCell = await ckb.getLiveCell(depositOutPoint.txHash)

The light client does not have the RPC named get_live_cell. Maybe it's better to get the live cell by another RPC method.

@Keith-CY
Copy link
Member

@homura
Copy link

homura commented Nov 16, 2023

The light client does not have the RPC named get_live_cell. Maybe it's better to get the live cell by another RPC method.

you're right, we should use the piece of the logic below, by using get_transaction function instead

Why not reuse the logic implemented at ckb-js/ckb-sdk-js@develop/packages/ckb-sdk-core/src/index.ts#L423-L452

@Keith-CY
Copy link
Member

The light client does not have the RPC named get_live_cell. Maybe it's better to get the live cell by another RPC method.

you're right, we should use the piece of the logic below, by using get_transaction function instead

Why not reuse the logic implemented at ckb-js/ckb-sdk-js@develop/packages/ckb-sdk-core/src/index.ts#L423-L452

Now it should be simple to fix because code snippets and test cases are ready, can we prioritize this fix in lumos so unlock nervos dao can be fixed by upgrading the SDK?

@homura
Copy link

homura commented Nov 17, 2023

Now it should be simple to fix because code snippets and test cases are ready, can we prioritize this fix in lumos so unlock nervos dao can be fixed by upgrading the SDK?

I've tried to fix the issue but I am struggling to verify it with the light client. It currently functions properly with the full node, but I'm unsure why the light client with devnet is unable to work with Neuron.

Screen.Recording.2023-11-17.at.17.01.39.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants