This repository has been archived by the owner on Aug 2, 2021. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Swap available balance #1892
Swap available balance #1892
Changes from all commits
f0431f4
de5b152
18500a9
d11b4b3
d4e5abc
1ff69ff
6eb0a57
523d220
075c322
7807019
e0cab6a
203dbec
f52e515
57416dd
4011ffe
80296ac
cacb39f
ed886ab
5bc0c28
c11f840
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
comment exported func
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.
Done!
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 would really recommend avoiding 2 blockchain wide event filters in one function, this can take quite a while on a real network (especially if using remote endpoints like infura). Instead we could compute this per peer using
s.contract.availableBalanceFor(peer) + s.contract.paidOut(p.beneficiary) - p.getLastSentCheque().CumulativeAmount
.If we need this for all peers (not sure why we would though) we can compute
liquidBalance() + SUM(s.contract.paidOut(peer)) - SUM(p.getLastSentCheque().CumulativeAmount)
. The last sum can be computed locally since we have all cheques in our store.If we implement it that way we also don't need
s.paidOut
.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.
Hi Ralph, this call is needed for a node operator to figure out how much balance is still left in his chequebook against which new cheques can be written. It allows him to figure out what his spendable balance is and when he has to re-deposit. Furthermore, this call is needed by Swarm in order to verify that an to-be-send cheque is not overspending.
I will see how and if I can incorporate your proposed formula. Thanks for the suggestion!
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.
Hi Ralph,
Based on your comment, I removed
paidOut
from theswap
structure and placeavailableBalance
there instead. This change minimizes the need to ever call theComputeAvailableBalance
function to one (boot-up).If we would go for implementing your (second) formula instead of how it is implemented now with the event filters, we need to do a call to the smart-contract to read paidOut for every peer.
Is this indeed quicker than the event filter? Note that Infura recently updated how they handle event queries: https://blog.infura.io/faster-logs-and-events-e43e2fa13773
If you think that what you propose is indeed much quicker, I will make the update!
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.
Note: the option you presented and the one which is currently implemented are not entirely the same. They deviate in the case where a cheque is sent outside of Swarm and already cashed. The current implementation would recognize this (and calculate the available balance correctly from the point of cashing onwards), while your proposal would never recognize this cheque as it is not cashed by a peer, hence the
availableBalance
will always be off.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 not consider the case with cheques outside of Swarm, this is always a user error and if done other things will break anyway. If we want the chequebook to be able to be used for other purposes we should introduce an RPC call for that so that the actual cheque creation and sending still takes place in the Swarm node.
Blockchain-wide event queries (from block 0 to "latest") can be really slow in my experience (Have you tested this call against ropsten or mainnet?). The Infura optimisation doesn't help much if it is then still slow for other users. (Since it's only being called once now it's not as big of a problem anymore).
While true that my formula does require more calls to
paidOut()
, this is aneth_call
operation against the latest state which is a very fast operation. My per-peer formula would also have the advantage that it correctly accounts for any set hard deposit.The current approach also doesn't update
availableBalance
correctly if the balance increases due to a cashed cheque after the node has started.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 would compute
availableBalance
on a per-peer basis whenever we want to send a cheque. Then it only requires twoeth_call
for every cheque sent.If we want this
availableBalance
for all peers we have to do aneth_call
for every peer but that would only happen when the RPC call is made and not during regular operation.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.
great discussion guys!
i don't understand the issue as in-depth as you two, but at a high level, it seems sensible to me to query the source of the data (in this case the blockchain) for the latest info when it's needed.
but this, of course, is contingent upon these queries not slowing down the system significantly. it might be hard to verify this.
is the alternative (proposed by @Eknir) slower overall? or is it just less robust/more complicated? i understand the query from the genesis block to the present is slow according to @ralph-pichler, but it would be less queries in the long run, right?
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 need
availableBalance
for all peers in order to know how far away we are from needing to make a new deposit, right?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.
@mortelli , it's an interesting discussion indeed. At this stage, I would personally favor robustness over efficiency, as we can always make efficiency gains later on, but robustness flaws are probably less nice to detect in a live network.
I think the alternative which I proposed was not necessarily slower, as it would only call to the blockchain upon boot-up. Subsequently, the idea was to keep a shadow view of the state of the blockchain, which requires little resources, but it is definitely less robust than calling the blockchain whenever needed.
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.
If I may put my thoughts here. I pretty much have to agree with @ralph-pichler.
As @ralph-pichler says these events are horribly slow. We've been using them in Giveth and at one point it took 1.5hs to reprocess everything (around 5000 events). And this is after optimizations like querying since the contract deployment etc. Sure here it will not be that extreme in most of the cases but we should at minimum test it. And yeah chain reorgs or connection issues created a lot of errors and had to be handled.
I also agree here. At least for the immediate future, I don't think we should be too bothered by this case. We should make an issue about this but not actively solve it now.
One of the usecases I can imagine is when user runs multiple swarm nodes with one chequebook. Still this brings a lot of problems to solve on the accounting side because it introduces all sorts of concurrency issues.
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.
This doesn't need to be changed in this PR but we really need to start thinking about contexts and timeouts when doing blockchain interaction. Here we call
PaidOut
in a loop, each call might take a while or never terminate at all. Can you open an issue for this in case there is none?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 will
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.
Please comment on the issue @ralph-pichler , with your recommendation for setting
context
. Issue: #1910