-
Notifications
You must be signed in to change notification settings - Fork 20
Fix links in Rewards module; migrate Transaction #935
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
Conversation
This closes issue #934.
carlostome
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Left some comments here and there. Some are related to the content of Transaction not the migration itself but I thought we could use the opportunity to improve it.
| + The hash of the serialized form of the transaction that was included | ||
| in the block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing what field this is talking about
| --> | ||
|
|
||
|
|
||
| ## Transaction Functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auxiliary functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better if the header of the section of each module where functions related to that module are defined should refer to the name of the module itself; we already do this elsewhere, e.g., "Certification Functions" (or "Certificates Functions"?), "Governance Functions", etc. Making the subsection titles more specific, less generic, seems preferable to me.
| \begin{code} | ||
| ``` | ||
|
|
||
| *Abstract types* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make these headings into subsubsections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... that might be a good idea. I'm not sure. We've always had these kinds of type labels, since the era of pdfs. Of course, that doesn't mean it's the right way to present them. But I wonder if it's best to keep record types contained in a single subsection, rather than spreading their fields across subsections. 🤔 That might be better for getting a "high level" understanding of the code. Though I suppose splitting a large record up into subsections with explanatory prose would make it easier to explain things at a "low level."
Well, I'll leave it for now and we can discuss whether improvements should be made in a future issue/PR.
| ``` | ||
| --> | ||
|
|
||
| *Derived types* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as previous comment
| ``` | ||
| --> | ||
|
|
||
| *Transaction types* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as previous comments
04e1c4c to
351e20a
Compare
plus final cleanup of prose Co-authored-by: Carlos Tomé Cortiñas <[email protected]>
351e20a to
021604b
Compare
Description
This closes issue #934.
Checklist
CHANGELOG.md