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: Add light client compatible and change to json. #2916

Merged
merged 3 commits into from
Nov 17, 2023

Conversation

yanguoyu
Copy link
Collaborator

No description provided.

@Keith-CY
Copy link
Collaborator

Keith-CY commented Oct 30, 2023

I have 2 points against this format.

  1. Many more fields should be updated when Neuron or CKB nodes have new release
    1.1 Neuron has a new release

    {
    +  "neuronVersions": ["0.112", "0.111", "0.110", "0.106", "0.103"],
      "fullVersions": ["0.111", "0.110", "0.109", "0.108", "0.107", "0.106", "0.105", "0.104", "0.103"],
      "lightVersions": ["0.2"],
      "compatible": {
    +    "0.112": {
    +      "full": ["0.111", "0.110", "0.109"],
    +      "light": ["0.2"]
    +    },
        "0.111": {
          "full": ["0.111", "0.110", "0.109"],
          "light": ["0.2"]
        },
        "0.110": {
          "full": ["0.111", "0.110", "0.109"],
          "light": ["0.2"]
        },
        "0.106": {
          "full": ["0.108", "0.107", "0.106", "0.105"],
          "light": []
        },
        "0.103": {
          "full": ["0.104", "0.103"],
          "light": []
        }
      }
    }

    1.2 CKB has new release

    {
      "neuronVersions": ["0.111", "0.110", "0.106", "0.103"],
    +  "fullVersions": ["0.112", "0.111", "0.110", "0.109", "0.108", "0.107", "0.106", "0.105", "0.104", "0.103"],
      "lightVersions": ["0.2"],
      "compatible": {
        "0.111": {
    +      "full": ["0.112", "0.111", "0.110", "0.109"],
          "light": ["0.2"]
        },
        "0.110": {
    +      "full": ["0.112", "0.111", "0.110", "0.109"],
          "light": ["0.2"]
        },
        "0.106": {
          "full": ["0.108", "0.107", "0.106", "0.105"],
          "light": []
        },
        "0.103": {
          "full": ["0.104", "0.103"],
          "light": []
        }
      }
    }

    The line to update depends on how many CKB@v0.111.0 exists.

  2. redundant info
    There are two fields representing versions of Neuron, I guess the standalone field neuronVersions is for the sequence of the version number, but it introduces redundancy.

@yanguoyu
Copy link
Collaborator Author

redundant info
There are two fields representing versions of Neuron, I guess the standalone field neuronVersions is for the sequence of the version number, but it introduces redundancy.

This file will be used for https://neuron.magickbase.com/ and Neuron to load the compatibility of Neuron. I guess it's better to simply the load and analysis process. Additionally, this file will auto-update and then be checked by developers. For the auto-update, adding more lines is not a problem.

@Keith-CY
Copy link
Collaborator

redundant info
There are two fields representing versions of Neuron, I guess the standalone field neuronVersions is for the sequence of the version number, but it introduces redundancy.

This file will be used for neuron.magickbase.com and Neuron to load the compatibility of Neuron. I guess it's better to simply the load and analysis process. Additionally, this file will auto-update and then be checked by developers. For the auto-update, adding more lines is not a problem.

I would like to know how to update the JSON file by a script, it may require going through the entire file.

And the field neuronVersions is duplicated with the key list of compatible except neuronVersions is ordered, but it does introduce redundant data.

@yanguoyu
Copy link
Collaborator Author

I would like to know how to update the JSON file by a script, it may require going through the entire file.

To update the JSON file, it needs to load the value of the file and generate a new value by the update type and version. Then write the updated value to file by cover.

And the field neuronVersions is duplicated with the key list of compatible except neuronVersions is ordered, but it does introduce redundant data.

That's true, it introduces redundant data. But it will be easy to iterate all of the Neuron versions. So this redundant is acceptable to me.

@Keith-CY
Copy link
Collaborator

Keith-CY commented Oct 30, 2023

I would like to know how to update the JSON file by a script, it may require going through the entire file.

To update the JSON file, it needs to load the value of the file and generate a new value by the update type and version. Then write the updated value to file by cover.

To add a new version of CKB, we should

  1. load the entire file
  2. get the latest version of ckb from fullVersions, 0.111 in this case
  3. filter neuron versions that include the latest version of ckb(0.111), 0.111 and 0.110 in this case
  4. prepend the new version into these arrays, fullVersions, compatible['0.111'] and compatible['0.110'] in this case.

However, updating the csv is simply as

awk -v ckb=$ckb 'BEGIN{FS=OFS=","} {$2 = ($2 ~ /[0-9]+\.[0-9]{3}/) ? ckb OFS $2 : $2 OFS $2} 1' compatible.csv

And the field neuronVersions is duplicated with the key list of compatible except neuronVersions is ordered, but it does introduce redundant data.

That's true, it introduces redundant data. But it will be easy to iterate all of the Neuron versions. So this redundant is acceptable to me.

Neuron versions can be iterated by Object.keys(json.compatible) except the order is determined by the runtime.

@WhiteMinds
Copy link
Contributor

Using CSV format can simplify the development cost of neuron compatibility data statistics, while using JSON format can simplify the development cost of developers who need to use neuron compatibility data.

I prefer using JSON because the development cost of generating and updating JSON scripts is paid once, and the development cost of reading is very low.
If it is CSV, additional CSV parsing is generally required when reading, as well as converting CSV data into a data structure that can be easily used (similar to our JSON structure), and each developer needs to develop this process once.

@Keith-CY
Copy link
Collaborator

Using CSV format can simplify the development cost of neuron compatibility data statistics, while using JSON format can simplify the development cost of developers who need to use neuron compatibility data.

I prefer using JSON because the development cost of generating and updating JSON scripts is paid once, and the development cost of reading is very low. If it is CSV, additional CSV parsing is generally required when reading, as well as converting CSV data into a data structure that can be easily used (similar to our JSON structure), and each developer needs to develop this process once.

Using CSV format can simplify the development cost of neuron compatibility data statistics, while using JSON format can simplify the development cost of developers who need to use neuron compatibility data.

I prefer using JSON because the development cost of generating and updating JSON scripts is paid once, and the development cost of reading is very low. If it is CSV, additional CSV parsing is generally required when reading, as well as converting CSV data into a data structure that can be easily used (similar to our JSON structure), and each developer needs to develop this process once.

I'm convinced by the reason write once complexly and read everywhere easily

@Keith-CY
Copy link
Collaborator

I'm convinced that complexity should be moved to the wirte process, but I still wonder why neuron versions are listed in an array and in compatible as its keys. IMO single data origin would be better

@WhiteMinds
Copy link
Contributor

but I still wonder why neuron versions are listed in an array and in compatible as its keys

This is indeed a convenience provided for a specific scenario (compatibility traversal of full/light based on neuron version).
Now I think it may not be suitable for all scenarios, because I am not sure if this will increase the difficulty of reading data in other scenarios.

@WhiteMinds
Copy link
Contributor

WhiteMinds commented Nov 6, 2023

Since this feature will block the implementation of Magickbase/websites#24, we need to advance it as soon as possible.

I think we can first use the structure implemented by this PR but not directly expose it as a stable data source until we think its structure is stable.

@Keith-CY @yanguoyu What do you think?

@yanguoyu
Copy link
Collaborator Author

yanguoyu commented Nov 6, 2023

Since this feature will block the implementation of Magickbase/websites#24, we need to advance it as soon as possible.

I think we can first use the structure implemented by this PR but not directly expose it as a stable data source until we think its structure is stable.

@Keith-CY @yanguoyu What do you think?

I guess after I remove the neuronVersions field, it can be merged. Any other questions about it? @Keith-CY

@Keith-CY
Copy link
Collaborator

Keith-CY commented Nov 6, 2023

Since this feature will block the implementation of Magickbase/websites#24, we need to advance it as soon as possible.
I think we can first use the structure implemented by this PR but not directly expose it as a stable data source until we think its structure is stable.
@Keith-CY @yanguoyu What do you think?

I guess after I remove the neuronVersions field, it can be merged. Any other questions about it? @Keith-CY

I would suggest removing the CSV along with adding the JSON, so there will be only one data source. Otherwise, another PR to remove the CSV is required before next release, or CSV and JSON should be updated simultaneously on next release.

@yanguoyu
Copy link
Collaborator Author

yanguoyu commented Nov 7, 2023

Since this feature will block the implementation of Magickbase/websites#24, we need to advance it as soon as possible.
I think we can first use the structure implemented by this PR but not directly expose it as a stable data source until we think its structure is stable.
@Keith-CY @yanguoyu What do you think?

I guess after I remove the neuronVersions field, it can be merged. Any other questions about it? @Keith-CY

I would suggest removing the CSV along with adding the JSON, so there will be only one data source. Otherwise, another PR to remove the CSV is required before next release, or CSV and JSON should be updated simultaneously on next release.

Neuron has not read the JSON file yet, I would remove the CSV file and read from JSON and add an updater in another PR.

@WhiteMinds
Copy link
Contributor

Neuron has not read the JSON file yet, I would remove the CSV file and read from JSON and add an updater in another PR.

@Keith-CY How do you think?

@Keith-CY
Copy link
Collaborator

Keith-CY commented Nov 8, 2023

Neuron has not read the JSON file yet, I would remove the CSV file and read from JSON and add an updater in another PR.

@Keith-CY How do you think?

The sentence is ambiguous, but remove csv, read from json by this PR and add updater in another PR would be good. Function would be broken if read from json is not supported when csv is missing

@yanguoyu yanguoyu force-pushed the feat-compatible-json branch 2 times, most recently from eb423b6 to 255ffd1 Compare November 16, 2023 06:03
.github/workflows/update_neuron_compatible.yml Outdated Show resolved Hide resolved
.github/workflows/update_neuron_compatible.yml Outdated Show resolved Hide resolved
.github/workflows/update_neuron_compatible.yml Outdated Show resolved Hide resolved
packages/neuron-wallet/src/services/node.ts Outdated Show resolved Hide resolved
scripts/update-ckb-client-versions.js Outdated Show resolved Hide resolved
scripts/update-neuron-compatible.js Outdated Show resolved Hide resolved
scripts/update-neuron-compatible.js Outdated Show resolved Hide resolved
scripts/update-neuron-compatible.js Outdated Show resolved Hide resolved
scripts/update-neuron-compatible.js Outdated Show resolved Hide resolved
scripts/update-neuron-compatible.js Outdated Show resolved Hide resolved
.github/workflows/update_neuron_compatible.yml Outdated Show resolved Hide resolved
@Keith-CY Keith-CY added this pull request to the merge queue Nov 17, 2023
Merged via the queue into nervosnetwork:develop with commit c69258f Nov 17, 2023
10 checks passed
@yanguoyu yanguoyu deleted the feat-compatible-json branch November 17, 2023 04:15
@Keith-CY Keith-CY mentioned this pull request Nov 23, 2023
@Keith-CY Keith-CY mentioned this pull request Dec 6, 2023
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.

None yet

5 participants