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

Transfer multiple inputs rebase #125

Merged
merged 26 commits into from
Dec 20, 2017

Conversation

TimDaub
Copy link
Contributor

@TimDaub TimDaub commented Nov 23, 2017

Rebase of #120

Don't know if working appropriately.

@codecov-io
Copy link

codecov-io commented Nov 23, 2017

Codecov Report

Merging #125 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #125      +/-   ##
==========================================
+ Coverage   82.26%   82.33%   +0.06%     
==========================================
  Files          21       21              
  Lines         282      283       +1     
==========================================
+ Hits          232      233       +1     
  Misses         50       50
Impacted Files Coverage Δ
src/transaction/makeTransferTransaction.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad03590...2348a49. Read the comment docs.

@future-is-present
Copy link
Contributor

As there are breaking changes, we will need to update everything. So I propose to handle this PR next week after we first create a new release of the JS driver including the metadata search: #124 for this week. So:

docs/Makefile Outdated
@@ -26,7 +26,19 @@ html:
@echo
@echo "Build finished. The HTML pages are in $(BUILDDIR)/html."

<<<<<<< dfe221031eab9e09c54fb4c8bf0feb54fd250043
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rm merge conflict

@future-is-present
Copy link
Contributor

future-is-present commented Dec 19, 2017

When creating a transfer transaction the user has to specify the 'transaction' and the 'output_index' that he/she is spending. For example:

Transaction.makeTransferTransaction(
        [{ tx: transferTx, output_index: 0 }],
        [aliceOutput],
        metaData
    )

Travis do not mark this as an error, but maybe we want to change to camelcase and also change 'tx' for 'transaction':

Transaction.makeTransferTransaction(
        [{ transaction: transferTx, outputIndex: 0 }],
        [aliceOutput],
        metaData
    )

@future-is-present
Copy link
Contributor

Anyways I would like to merge this PR. Is needed for sub-divisible assets. Can you @jernejpregelj check it?

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.

9 participants