Skip to content
This repository has been archived by the owner on Aug 27, 2024. It is now read-only.

refining off-chain indexing #800

Merged
merged 10 commits into from
Dec 14, 2020
Merged

refining off-chain indexing #800

merged 10 commits into from
Dec 14, 2020

Conversation

jimmychu0807
Copy link
Contributor

@jimmychu0807 jimmychu0807 commented Dec 11, 2020

@tomusdrw @danforbes

I have:

  • Combined the topic of off-chain indexing under off-chain workers.
  • Refined the write-up somewhat (tomek, let me know if all you want to say originally are still there is no mis-interpretation.
  • I don't think this section is good to have rust code shown up (we are teaching Substrate fundamental concepts here). In fact I think it worth to have another recipe to demonstrating using off-chain indexing. So that part is removed, and a new recipe is queued to be written on this topic.

Let me know what you think. Thanks

Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm, although I think we need to point to a code sample to make the example relevant, now it doesn't really help anyone to get started with Off-Chain Indexing (and that was my goal with the original example).

be exactly the same for every node with indexing enabled.

Off-chain database can be read using remote procedure calls (RPC) so it fits the use case of storing
indefinitely growing data without over-consuming the on-chain storage.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe let's add the code samples to Development Guide as well - right now, since there is no pointers at all it's really hard to go from "I've learned the concept" to "I want to try it out".

Copy link
Contributor

@danforbes danforbes Dec 11, 2020

Choose a reason for hiding this comment

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

I would prefer to follow Jimmy's suggestion and keep the code in the Recipes.

Edit: this definitely implies we need to make these links easy to find and follow 💯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks input from Tomek and Dan.

Tomek, I commit that I will write a sample code, either in recipe or the Runtime section below on off-chain index before Jan 15th. If we put code out, we likely need a section of code and explain to readers the context. So I need to experiment with the API myself first, and this takes some work and time. Will let you guys review it once it is done. Then we will add a link to the code section here.

So I prefer keeping this section to be conceptual and show the code in another section. Let me know if you are okay with this, @tomusdrw

@@ -46,3 +46,23 @@ should be implemented to determine what information gets into the chain.

For more information on how to use off-chain workers in your next runtime development project,
please refer to our [Development Guide](../runtime/off-chain-workers).

# Off-Chain Indexing
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I'm not sure it's fair to bundle Off-Chain Indexing under Off-Chain Workers. These two things can really be though of independently.

I'd suggest renaming the entire article to "Off-Chain Features" or sth, and then having 3 sections that outline 3 independent substrate features:

  • Off-Chain Database - a node-local database that every substrate node has, can be accessed over RPC (both writes (unsafe) and reads (safe)).
  • Off-Chain Workers - an idea of running off-chain an arbitrary logic defined within the runtime code (when the node is fully synced). Workers can access Off-Chain DB.
  • Off-Chain Indexing - a Runtime function to populate Off-Chain DB (opt-in via CLI flag) even during sync.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you both for thinking critically about how we present these concepts to our users 🙏 I would not be opposed to Tomek's suggestion at all, and we could even think about "lifting" this out of the Runtime section and creating a new top-level Off-Chain section or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will follow tomek suggestion of renaming the section as Off-Chain Features and having 3 children sections inside.

Copy link
Contributor

@danforbes danforbes left a comment

Choose a reason for hiding this comment

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

Thank you both 🙏

docs/knowledgebase/learn-substrate/off-chain-workers.md Outdated Show resolved Hide resolved
docs/knowledgebase/learn-substrate/off-chain-workers.md Outdated Show resolved Hide resolved
@@ -46,3 +46,23 @@ should be implemented to determine what information gets into the chain.

For more information on how to use off-chain workers in your next runtime development project,
please refer to our [Development Guide](../runtime/off-chain-workers).

# Off-Chain Indexing
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you both for thinking critically about how we present these concepts to our users 🙏 I would not be opposed to Tomek's suggestion at all, and we could even think about "lifting" this out of the Runtime section and creating a new top-level Off-Chain section or something.

be exactly the same for every node with indexing enabled.

Off-chain database can be read using remote procedure calls (RPC) so it fits the use case of storing
indefinitely growing data without over-consuming the on-chain storage.
Copy link
Contributor

@danforbes danforbes Dec 11, 2020

Choose a reason for hiding this comment

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

I would prefer to follow Jimmy's suggestion and keep the code in the Recipes.

Edit: this definitely implies we need to make these links easy to find and follow 💯

@jimmychu0807
Copy link
Contributor Author

@tomusdrw as you suggested, I have rewritten the section and rename it to Off-Chain Features. Let me know what you think and if they are still technically correct 🙏

Thank you all for the review, tomek and dan.

Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

❤️ looks great, just small clarification about on-chain logic access to off-chain storage would be good.

docs/knowledgebase/learn-substrate/off-chain-features.md Outdated Show resolved Hide resolved
docs/knowledgebase/learn-substrate/off-chain-features.md Outdated Show resolved Hide resolved
Copy link
Contributor

@danforbes danforbes left a comment

Choose a reason for hiding this comment

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

Can you guys please review my changes and make sure I didn't mess anything up? f0db1ab This is a great addition...thank you so much, @tomusdrw and @jimmychu0807 🙏🏻

@jimmychu0807 jimmychu0807 merged commit 5fbc893 into source Dec 14, 2020
@jimmychu0807 jimmychu0807 deleted the jc/off-chain-indexing branch December 14, 2020 14:16
CurlyBracketEffect pushed a commit to CurlyBracketEffect/substrate-developer-hub.github.io that referenced this pull request Jan 14, 2021
* refining off-chain indexing

* Update docs/knowledgebase/learn-substrate/off-chain-workers.md

Co-authored-by: Dan Forbes <[email protected]>

* Update docs/knowledgebase/learn-substrate/off-chain-workers.md

Co-authored-by: Dan Forbes <[email protected]>

* Expanded the section from off-chain-worker to off-chain-features

* Update docs/knowledgebase/learn-substrate/off-chain-features.md

Co-authored-by: Tomasz Drwięga <[email protected]>

* update on content to make it more coherent

* Fix link

* Links and nitpicks

* Prettier

Co-authored-by: Dan Forbes <[email protected]>
Co-authored-by: Tomasz Drwięga <[email protected]>
Co-authored-by: Dan Forbes <[email protected]>
joepetrowski added a commit that referenced this pull request May 14, 2021
* Add validate address options

I have added the 2 main tools that Parity has created to validate ss58 addresses. I also added a list of places for developers to look for some more code examples to use to create their own custom validation tools in various languages.

@joepetrowski @danforbes

* Fix line length

* Add validate address options

I have added the 2 main tools that Parity has created to validate ss58 addresses. I also added a list of places for developers to look for some more code examples to use to create their own custom validation tools in various languages.

@joepetrowski @danforbes

* Nitpicks

* Updated Terms of Use per Legal (#803)

* Updated Terms of Use per Legal

* Prettier

`npx prettier --write --print-width 100 ./website/pages/en/terms.js`

Co-authored-by: Dan Forbes <[email protected]>

* refining off-chain indexing (#800)

* refining off-chain indexing

* Update docs/knowledgebase/learn-substrate/off-chain-workers.md

Co-authored-by: Dan Forbes <[email protected]>

* Update docs/knowledgebase/learn-substrate/off-chain-workers.md

Co-authored-by: Dan Forbes <[email protected]>

* Expanded the section from off-chain-worker to off-chain-features

* Update docs/knowledgebase/learn-substrate/off-chain-features.md

Co-authored-by: Tomasz Drwięga <[email protected]>

* update on content to make it more coherent

* Fix link

* Links and nitpicks

* Prettier

Co-authored-by: Dan Forbes <[email protected]>
Co-authored-by: Tomasz Drwięga <[email protected]>
Co-authored-by: Dan Forbes <[email protected]>

* Bump ini from 1.3.5 to 1.3.7 in /website (#801)

Bumps [ini](https://github.com/isaacs/ini) from 1.3.5 to 1.3.7.
- [Release notes](https://github.com/isaacs/ini/releases)
- [Commits](npm/ini@v1.3.5...v1.3.7)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jimmy Chu <[email protected]>

* Making most static strings in substrate.dev supporting multi-lingual (#802)

* adding <translate> tag for multi-lingual support

* Most strings are translatable now

* Minor fix of bash syntax in console output

* fixing spacing

* using relative link so multi-lingual option is kept between navigation

* cmd style in console output

* Indent all JS to use spacing for indentation

* Updated to latest libs

* Fix site internal links to respect multi-lingual.

* Add validate address options

I have added the 2 main tools that Parity has created to validate ss58 addresses. I also added a list of places for developers to look for some more code examples to use to create their own custom validation tools in various languages.

@joepetrowski @danforbes

* Fix line length

* Add validate address options

I have added the 2 main tools that Parity has created to validate ss58 addresses. I also added a list of places for developers to look for some more code examples to use to create their own custom validation tools in various languages.

@joepetrowski @danforbes

* Fix: Comment Edits

* Update docs/knowledgebase/advanced/ss58-address-format.md

Co-authored-by: joe petrowski <[email protected]>

* Update docs/knowledgebase/advanced/ss58-address-format.md

Co-authored-by: joe petrowski <[email protected]>

* Update docs/knowledgebase/advanced/ss58-address-format.md

Co-authored-by: joe petrowski <[email protected]>

* Update docs/knowledgebase/advanced/ss58-address-format.md

Co-authored-by: joe petrowski <[email protected]>

* Update docs/knowledgebase/advanced/ss58-address-format.md

Co-authored-by: joe petrowski <[email protected]>

* Review and edits for SS58 

General pruning to make text more succinct. Slight tweak on structure and linking to be more consistent with other KB articles. Added code comments too.

* Revert "Review and edits for SS58 "

This reverts commit 532d283.

* Apply uncontroversial suggestions from code review

Co-authored-by: sacha-l <[email protected]>

* fix github suggestions error

* Update docs/knowledgebase/advanced/ss58-address-format.md

Co-authored-by: sacha-l <[email protected]>

* fix: wording and formatting

* Update docs/knowledgebase/advanced/ss58-address-format.md

Co-authored-by: Dan Forbes <[email protected]>
Co-authored-by: Imad Arain <[email protected]>
Co-authored-by: Jimmy Chu <[email protected]>
Co-authored-by: Dan Forbes <[email protected]>
Co-authored-by: Tomasz Drwięga <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jimmy Chu <[email protected]>
Co-authored-by: joe petrowski <[email protected]>
Co-authored-by: Sacha <[email protected]>
Co-authored-by: sacha-l <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants