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

docs: Add documentation about errors (codespace and codes) #54

Merged
merged 2 commits into from
Jun 28, 2023

Conversation

ryu1-sakai
Copy link

@ryu1-sakai ryu1-sakai commented Jun 26, 2023

Description

Adds documentation about errors (codespace and codes) caused by wasm.

Motivation and context

Errors (codespace and codes) are part of the interface between servers (mainnet nodes) and clients.
So their specs need to be clearly defined and described in somewhere shared by both sever and client developers, such as protocols, official documents, etc.

How has this been tested?

Confirmed that make install succeeded.

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have updated API documentation client/docs/swagger-ui/swagger.yaml

@ryu1-sakai ryu1-sakai force-pushed the docs/add-document-about-errors branch from bb39f2d to ca2ff8b Compare June 26, 2023 14:20
da1suk8
da1suk8 previously approved these changes Jun 27, 2023
@loloicci
Copy link

Thank you for this work, @ryu1-sakai !
Could you write where (what part of code) these errors information comes from, if it is clear and collected in a file?

Copy link
Member

@zemyblue zemyblue 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 for your contribution.
Please add a link to the table of contents at the top of this x/wasm/README.md.

@ryu1-sakai
Copy link
Author

@loloicci, the information of the documentation was taken from the following code.
https://github.com/Finschia/wasmd/blob/0a1d4576947632cf00f7dba16c564657079debe5/x/wasm/types/errors.go

@zemyblue, I added the link in the table of contents.
0faf83d

@loloicci
Copy link

@ryu1-sakai

the information of the documentation was taken from the following code.
https://github.com/Finschia/wasmd/blob/0a1d4576947632cf00f7dba16c564657079debe5/x/wasm/types/errors.go

Thank you. How about adding the info on this file path to README?

@ryu1-sakai
Copy link
Author

ryu1-sakai commented Jun 28, 2023

@loloicci

How about adding the info on this file path to README?

I think it's beter not to mention errors.go here.

What should be written here is external specifications, not internal designs/implementations. The content added by this PR explains external specifications about errors, which both servers (nodes in case of Finschia) and clients need to know.

On the other hand, errors.go is part of internal implementations. Internal implementations may be modified as part of refactoring (without changes in external specifications) in the future. So it's better NOT to mention internal implementations in the document to avoid necessity to also modify documents when we refactor internal implementations.

Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

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

LGTM

@loloicci
Copy link

OK, then now we will add error info to README, and after Finschia/finschia-sdk#942 is solved, we will unify the way to manage error info with finschia.

@zemyblue zemyblue added the documentation Improvements or additions to documentation label Jun 28, 2023
@loloicci loloicci merged commit 881c77f into Finschia:main Jun 28, 2023
@zemyblue zemyblue mentioned this pull request Aug 31, 2023
@zemyblue zemyblue mentioned this pull request Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants