-
Notifications
You must be signed in to change notification settings - Fork 9
Conversation
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.
Left some thoughts inline. I think we should combine our branches.
require.NoError(t, err) | ||
} | ||
|
||
// TODO - how does Initiator submit "latest", is it store in the Channel? |
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.
Good question, I think we'll store in channel, or rather the tx can regenerate the tx and add the signatures.
# Conflicts: # sdk/state/state.go
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 left a couple comments. I still need to look at the payment functions.
sdk/state/state.go
Outdated
Balance int64 | ||
Asset Asset |
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.
You can use an Amount
type here, it captures both the asset and amount value in the struct. That'll make it easier for us to add support for multiple balances.
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.
proposal to name the struct Balance
instead of Amount
? channel.amount.Amount
feels weird, and a "balance" for each asset makes sense
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.
Couple more thoughts.
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.
One more:
thanks for the reviews and comments @leighmcculloch . I addressed the above points, lmk what you think |
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 we should merge this along with #52 and then address inconsistencies because things probably need changing in both. I can take addressing those things in a new PR and you can review.
amountFromResponder = p.Amount | ||
} | ||
newBalance := c.balance.Amount + amountFromInitiator - amountFromResponder | ||
|
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.
We should validate the iteration number in the payment is what we expect too.
Where does the iteration number get incremented?
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.
right now it's living on the Payment, so iterated by the person sending the payment. I created a ticket to maybe do differently here
#61
updating the payment channel with new payment + confirming that payment