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

Update intro.md #2975

Merged
merged 5 commits into from
Feb 4, 2020
Merged

Update intro.md #2975

merged 5 commits into from
Feb 4, 2020

Conversation

rsoltanzadeh
Copy link
Contributor

Significant clarifications, especially in the paragraph where excess value is introduced and the section about kernel offsets.

Significant clarifications, especially in the paragraph where excess value is introduced and the section about kernel offsets.
doc/intro.md Outdated Show resolved Hide resolved
@antiochp
Copy link
Member

antiochp commented Aug 3, 2019

Looks good - I need to have another read through but 👍

"Splitting" the blinding factor into *k1* and *k2* is unnecessarily confusing I think. The excess and the offset are really two arbitrary numbers without any mathematical relation to each other. I think I rephrased the section as well as I could without sacrificing any correctness for the sake of simplicity (which I think would be more confusing than helpful).
Transactions are not aggregated by non-mining nodes as well. Removed the part about miners.
@sesam
Copy link
Contributor

sesam commented Dec 13, 2019

Really nice that you're working on improving the docs. This is really important for grin, IMHO.

I see the changed document has different column wrap. If you have time to rebase to get rid of the conflict that is currently preventing merging, maybe you could also have your text editor redo the line-breaks to match the original more narrow? I'm thinking that if the changes were easier to see, then this update could be easier to get through the review process.

@quentinlesceller
Copy link
Member

Just fixed the merge conflict. Going to have a thorough review today.

Copy link
Member

@quentinlesceller quentinlesceller 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 the pr @rsoltanzadeh! Looking good 👍. I'm approving the PR even if there is a few things that I'd like to discuss (see comment below) but nothing blocking as is.

doc/intro.md Show resolved Hide resolved
doc/intro.md Show resolved Hide resolved
doc/intro.md Show resolved Hide resolved
doc/intro.md Show resolved Hide resolved
@quentinlesceller
Copy link
Member

Okay LGTM.

@quentinlesceller quentinlesceller merged commit a41965e into mimblewimble:master Feb 4, 2020
@rsoltanzadeh rsoltanzadeh deleted the patch-1 branch February 7, 2020 17:31
@antiochp antiochp mentioned this pull request Feb 24, 2020
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.

4 participants