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.

Sign up for GitHub

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

Jump to bottom

Add Transaction Command #120

Merged
merged 10 commits into from
Aug 13, 2017
Merged

Add Transaction Command #120

merged 10 commits into from
Aug 13, 2017

Conversation

chadlagore
Copy link
Contributor

@chadlagore chadlagore commented Aug 10, 2017 •
edited

Status: Open for visibility

Related Issue

101

Description

  • Implementing the add transaction functionality.
  • Has some wide ranging effects: Transactions, Wallets, console etc.
  • Adjusted the transaction recipient to only use checksums instead of Address.
  • We may need to do the same with the transaction sender ☝️
  • Broadcast new transactions to other nodes.
What would you like to create?
   Wallet
 ❯ Transaction
💳  Enter recipient wallet address
>>> ef653f0f6f292b7a12392686ea03058045ad51c1
💵  Enter amount to send:
>>> 15
📬  Its in the mail!
>>> _

WIKI Updates

Todos

  • Initialize balance.
  • Test new Wallet methods.
  • Update the Wallet when new blocks come in to kill pending transactions.
  • Broadcast transactions to other nodes
  • Update own pool.

General:

  • Tests
  • Wiki

@chadlagore chadlagore mentioned this pull request Aug 10, 2017
Open
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.3%) to 68.58% when pulling 8c6eb26 on 101-add-transaction-command into b6d5e37 on dev.

@chadlagore chadlagore changed the title added transaction creation at console; removed hotwallet Add Transaction Command Aug 10, 2017
jordanschalm
jordanschalm reviewed Aug 10, 2017
View reviewed changes
Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

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

Really like the create transaction flow. We need to consider how we compute wallet balances, but we can leave that to a different PR. It's quite a large task and out of scope of this issue.

app/user.go
func getCurrentUser() *User {
// TODO: Check for local user information on disk,
// If doesnt exist, create new user.
return NewUser()
}

// Pay pays an amount of coin to an address `to`.
func (a *App) Pay(to string, amount uint64) error {
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to make some changes to this function.

We should:

  1. Create a signed transaction
  2. Broadcast it to all connected nodes

Independently of this function, the balance of the current user's wallet should be computed each time a new block is added to our chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, so the wallet balance is only updated when our transaction has landed in a block - makes total sense.

blockchain/wallet.go Outdated
Public() Address
Sign(digest Hash, random io.Reader) (Signature, error)
Debit(amount uint64) error
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above, I don't think we should modify a user account directly, it should be a function of the blockchain and should be recomputed each time we add a block to the chain.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.6%) to 68.285% when pulling e8c8e72 on 101-add-transaction-command into b6d5e37 on dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.1%) to 69.71% when pulling 90e8ccc on 101-add-transaction-command into b6d5e37 on dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 69.584% when pulling 19ddb85 on 101-add-transaction-command into b6d5e37 on dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 69.584% when pulling 284cd0f on 101-add-transaction-command into b6d5e37 on dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 71.394% when pulling 33557a3 on 101-add-transaction-command into b6d5e37 on dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 70.916% when pulling f12ebbc on 101-add-transaction-command into b6d5e37 on dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 70.916% when pulling f12ebbc on 101-add-transaction-command into b6d5e37 on dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 71.275% when pulling 7b02913 on 101-add-transaction-command into b6d5e37 on dev.

jordanschalm
jordanschalm approved these changes Aug 13, 2017
View reviewed changes
Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

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

Looking good

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 71.275% when pulling f06601d on 101-add-transaction-command into b6d5e37 on dev.

@chadlagore chadlagore merged commit 25241c2 into dev Aug 13, 2017
bfbachmann
bfbachmann reviewed Aug 13, 2017
View reviewed changes
app/user.go
// The transaction must be signed.
if txn, err := tbody.Sign(*a.CurrentUser.Wallet, crand.Reader); err == nil {

// The transaction must be broadcasted to the peers.
Copy link
Member

Choose a reason for hiding this comment

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

The comment here doesn't fit what's happening

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed up in inputs PR 😅

bfbachmann
bfbachmann reviewed Aug 13, 2017
View reviewed changes
blockchain/wallet.go
func (w *Wallet) GetEffectiveBalance() uint64 {
r := w.Balance
for _, t := range w.PendingTxns {
r -= t.GetTotalOutput()
Copy link
Member

Choose a reason for hiding this comment

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

Is this mathematically correct? Won't part of the output to each transaction go back to the sender, meaning the total output is not actually how much you're spending?

@jordanschalm jordanschalm deleted the 101-add-transaction-command branch August 30, 2017 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bfbachmann bfbachmann bfbachmann left review comments

@jordanschalm jordanschalm jordanschalm approved these changes

@david-julien david-julien Awaiting requested review from david-julien

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@chadlagore @coveralls @bfbachmann @jordanschalm