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

Add atomic swaps to docs #967

Closed
wants to merge 0 commits into from
Closed

Add atomic swaps to docs #967

wants to merge 0 commits into from

Conversation

imestin
Copy link
Contributor

@imestin imestin commented Jul 13, 2019

Closes #964

@s-ben
Copy link
Contributor

s-ben commented Jul 14, 2019

@imestin, first pass it looking good. A few suggested changes.

  • This PR is from your local master branch. Typically when you make changes, you want to create a local feature branch from master first (e.g. add-atomic-swap), then submit a the PR from that feature branch. This is common practice and makes it easier to keep track of branches. Also, while I was able to pull this and get it running locally, I think it wrote over my local master branch, causing a little mess. Not hard to fix, but just something to keep in mind.
  • Your changes are spread across multiple commits. This is usually OK when iterating on changes with reviewers, or when there are only a couple commits. But generally, if you create a number of commits while working on something (common), you'll want to git squash your commits down to a single commit before submitting the PR. This makes it easier to review, as once this is in a single commit, I can use the Review feature to comment directly on lines. @jholdstock is this correct policy?
  • The atomic-swap page needs to be added to the navigation by updating mkdocs.yaml

Later, Atomic Swaps will be integrated in the Decredition GUI wallet.

It’s generally a bad idea to say things will be integrated in the future, even when we’re almost certain they will. Docs should only document what already exists. Otherwise, either a) it doesn’t get integrated somehow and the docs are wrong, or (more likely), b) it does get integrated but we forget to update this page, making people think it’s not integrated yet.

The first atomic swap happened between Decred and Litecoin on September 20, 2017.

Is there an interesting link we can insert here?

For a decentralized exchange to work, Lightning Network is also necessary. 

Can you say why this is true? Not absolutely necessary, just curious, and so might some other readers.

@jholdstock
Copy link
Member

Hi @imestin. Can you swap these over please?

Screenshot from 2019-07-14 09-45-38

@jholdstock
Copy link
Member

Its best to use common sense when structuring your PR commits. Sometimes squashing down to fewer commits can be helpful, sometimes it can hide important details.

If you are able to break the PR down into multiple commits which make sense on their own, and can be reviewed in isolation, thats good. Smaller chunks are good.

If you have a bunch of commits like "fix this" "fix that" "oops, revert previous", "try again" - this is just noise and does not help anybody.

@imestin imestin changed the title Closes #964 Add atomic swaps to docs Jul 15, 2019
@imestin
Copy link
Contributor Author

imestin commented Jul 15, 2019

Thank you for feedback, tomorrow I will correct the errors.

@imestin
Copy link
Contributor Author

imestin commented Jul 16, 2019

Can you say why this is true? Not absolutely necessary, just curious, and so might some other readers.

Yes, you are right, that was a bad assumption from me, other tools can do this as well.

imestin added a commit to imestin/dcrdocs that referenced this pull request Jul 16, 2019
imestin added a commit to imestin/dcrdocs that referenced this pull request Jul 16, 2019
@imestin
Copy link
Contributor Author

imestin commented Jul 16, 2019

I created another pull request that only has 1 commit and fixes the above errors.
#968

@imestin imestin closed this Jul 16, 2019
imestin added a commit to imestin/dcrdocs that referenced this pull request Jul 18, 2019
imestin added a commit to imestin/dcrdocs that referenced this pull request Jul 25, 2019
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.

Add atomic swaps to docs
3 participants