feat: Add light client compatible and change to json. by yanguoyu · Pull Request #2916 · nervosnetwork/neuron

yanguoyu · 2023-10-27T07:47:48Z

No description provided.

Keith-CY · 2023-10-30T01:31:13Z

I have 2 points against this format.

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.

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 · 2023-10-30T02:04:02Z

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 · 2023-10-30T03:51:41Z

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 · 2023-10-30T06:26:01Z

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 · 2023-10-30T08:01:25Z

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

load the entire file
get the latest version of ckb from fullVersions, 0.111 in this case
filter neuron versions that include the latest version of ckb(0.111), 0.111 and 0.110 in this case
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 · 2023-10-30T08:16:45Z

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 · 2023-10-30T08:27:11Z

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 · 2023-10-31T02:46:35Z

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 · 2023-10-31T03:04:38Z

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 · 2023-11-06T09:39:38Z

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 · 2023-11-06T10:01:56Z

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 · 2023-11-06T13:34:50Z

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 · 2023-11-07T00:26:39Z

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 · 2023-11-08T07:52:26Z

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 · 2023-11-08T08:49:26Z

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 requested review from Danie0918, Keith-CY, WhiteMinds, devchenyan and homura October 27, 2023 07:47

yanguoyu force-pushed the feat-compatible-json branch from 1e11df7 to 99437fb Compare October 27, 2023 07:50

WhiteMinds approved these changes Oct 27, 2023

View reviewed changes

devchenyan approved these changes Oct 29, 2023

View reviewed changes

yanguoyu force-pushed the feat-compatible-json branch from 11fb9c0 to e5239be Compare November 15, 2023 00:35

homura reviewed Nov 15, 2023

View reviewed changes

Comment thread compatible.json

yanguoyu force-pushed the feat-compatible-json branch 2 times, most recently from eb423b6 to 255ffd1 Compare November 16, 2023 06:03

Keith-CY reviewed Nov 16, 2023

View reviewed changes

yanguoyu added 2 commits November 16, 2023 20:35

feat: Add light client compatible and change to json.

52a111d

fix: update compatible.json as fs.write

60c6b0e

yanguoyu force-pushed the feat-compatible-json branch from 255ffd1 to 6d10107 Compare November 16, 2023 12:35

yanguoyu requested review from Keith-CY and homura November 16, 2023 12:35

Keith-CY approved these changes Nov 16, 2023

View reviewed changes

Comment thread .github/workflows/update_neuron_compatible.yml Outdated

fix: Use compatible.json and add action to update it.

f8e4730

yanguoyu force-pushed the feat-compatible-json branch from 6d10107 to f8e4730 Compare November 17, 2023 02:34

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

yanguoyu deleted the feat-compatible-json branch November 17, 2023 04:15

Keith-CY mentioned this pull request Nov 23, 2023

Release v0.112.0 #2956

Merged

Keith-CY mentioned this pull request Dec 6, 2023

Release v0.112.0 #2973

Merged

Uh oh!

Conversation

yanguoyu commented Oct 27, 2023

Uh oh!

Keith-CY commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanguoyu commented Oct 30, 2023

Uh oh!

Keith-CY commented Oct 30, 2023

Uh oh!

yanguoyu commented Oct 30, 2023

Uh oh!

Keith-CY commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WhiteMinds commented Oct 30, 2023

Uh oh!

Keith-CY commented Oct 30, 2023

Uh oh!

Keith-CY commented Oct 31, 2023

Uh oh!

WhiteMinds commented Oct 31, 2023

Uh oh!

WhiteMinds commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanguoyu commented Nov 6, 2023

Uh oh!

Keith-CY commented Nov 6, 2023

Uh oh!

yanguoyu commented Nov 7, 2023

Uh oh!

WhiteMinds commented Nov 8, 2023

Uh oh!

Keith-CY commented Nov 8, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Keith-CY commented Oct 30, 2023 •

edited

Loading

Keith-CY commented Oct 30, 2023 •

edited

Loading

WhiteMinds commented Nov 6, 2023 •

edited

Loading