Skip to content

refactor: replace toUint64Le, parseEpoch from ckbCore.utils with lumos - #3138

Merged
Keith-CY merged 1 commit into
nervosnetwork:developfrom
twhy:remove-ckbCore-exported-toUint64Le-parseEpoch
May 6, 2024
Merged

refactor: replace toUint64Le, parseEpoch from ckbCore.utils with lumos#3138
Keith-CY merged 1 commit into
nervosnetwork:developfrom
twhy:remove-ckbCore-exported-toUint64Le-parseEpoch

Conversation

@twhy

@twhy twhy commented Apr 30, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@homura

homura commented Apr 30, 2024

Copy link
Copy Markdown
Contributor

While refactoring, it is better to keep the ckbCore as it is to let the reviewer know that all the uint64-related encoding has been completed

declare const originalCkbCore;

export const ckbCore = {
  someMethod: originalCkbCore.someMethod.bind(originalCkbCore),
  // ...
  utils: {
    toUint64() {
      // return ...
    }
  }
}

@twhy

twhy commented Apr 30, 2024

Copy link
Copy Markdown
Contributor Author

While refactoring, it is better to keep the ckbCore as it is to let the reviewer know that all the uint64-related encoding has been completed

declare const originalCkbCore;

export const ckbCore = {
  someMethod: originalCkbCore.someMethod.bind(originalCkbCore),
  // ...
  utils: {
    toUint64() {
      // return ...
    }
  }
}

@homura After checking the code, I realized we probably don't need to add a proxy ckbCore to get this refactor done.

The ckbCore object is still there. This screenshot shows the changes more clearly.
And all uint64-related encoding has been completed in this PR.

image

@twhy

twhy commented May 6, 2024

Copy link
Copy Markdown
Contributor Author

@Keith-CY Let's merge this.

@Keith-CY
Keith-CY added this pull request to the merge queue May 6, 2024
Merged via the queue into nervosnetwork:develop with commit 4aea8d0 May 6, 2024
@twhy
twhy deleted the remove-ckbCore-exported-toUint64Le-parseEpoch branch May 6, 2024 02:20
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.

5 participants