Add IgnoreSenderReserveRequirements param
#3751
Conversation
|
Note that we currently check if only the deposit covers the ticket face value. We could instead check if the deposit+reserve covers the ticket value. |
|
Considering shortening |
Yeah, I think |
pm/recipient.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if deposit.Cmp(faceValue) < 0 { |
There was a problem hiding this comment.
Don't you need to take into account the current balance like it's done here?
stronk-dev
force-pushed
the
md/feature/allow_insecure_sender_reserve
branch
from
d0a7fa5 to
c6aa153
Compare
There was a problem hiding this comment.
Thanks for the PR Marco!
Added some comments, other than that, one more question: Don't you also need to modify validateSender()? Otherwise it may fail there with the "no sender reserve" error.
| cfg.MaxTotalEV = fs.String("maxTotalEV", *cfg.MaxTotalEV, "The maximum acceptable expected value for one PM payment") | ||
| // Broadcaster deposit multiplier to determine max acceptable ticket faceValue | ||
| cfg.DepositMultiplier = fs.Int("depositMultiplier", *cfg.DepositMultiplier, "The deposit multiplier used to determine max acceptable faceValue for PM tickets") | ||
| cfg.IgnoreSenderReserve = fs.Bool("IgnoreSenderReserve", *cfg.IgnoreSenderReserve, "Skip sender reserve validation and rely solely on broadcaster deposits covering ticket face value (unsafe)") |
There was a problem hiding this comment.
| cfg.IgnoreSenderReserve = fs.Bool("IgnoreSenderReserve", *cfg.IgnoreSenderReserve, "Skip sender reserve validation and rely solely on broadcaster deposits covering ticket face value (unsafe)") | |
| cfg.IgnoreSenderReserve = fs.Bool("ignoreSenderReserve", *cfg.IgnoreSenderReserve, "Ignore sender reserve; lowers gateway reserve needs but allows double-spending risk (unsafe)") |
| return nil, err | ||
| } | ||
| var maxFloat *big.Int | ||
| if !r.cfg.IgnoreSenderReserve { |
There was a problem hiding this comment.
Nit: how about revering the condition and check here if r.cfgSenderReserve and then have the logic for ignoring here and the rest in the else statement. Super nit, but it's harder to read when you have two reverse conditions (not and ignore, which effectively means if use sender reserve).
| monitor.TicketFaceValue(sender.Hex(), faceValue) | ||
| monitor.MaxFloat(sender.Hex(), maxFloat) | ||
| if !r.cfg.IgnoreSenderReserve && maxFloat != nil { | ||
| monitor.MaxFloat(sender.Hex(), maxFloat) |
There was a problem hiding this comment.
Ok, so we monitor this metric only fi the IgnoreSenerReserve is false (default). Is that correct? I think it should be ok, but just want to confirm that's the intention.
| totalReserve := new(big.Int).Set(info.Reserve.FundsRemaining) | ||
| if info.Reserve.ClaimedInCurrentRound != nil { | ||
| totalReserve.Add(totalReserve, info.Reserve.ClaimedInCurrentRound) | ||
| } | ||
|
|
||
| available := new(big.Int).Set(totalReserve) | ||
| if info.Deposit != nil { | ||
| available.Add(available, info.Deposit) | ||
| } | ||
| available.Sub(available, sm.senders[addr].pendingAmount) |
There was a problem hiding this comment.
Could you add a comment here or in the function comment explaining the calculations here?
I don't exactly get it, so the sender funds are calculated as:
funds = deposit + reserve + claimed in current round in pending amount
I think the most tricky part is why we add this claimed in current round.
| } | ||
|
|
||
| // SenderFunds returns the sender's deposit plus total reserve minus pending tickets | ||
| func (sm *LocalSenderMonitor) SenderFunds(addr ethcommon.Address) (*big.Int, error) { |
There was a problem hiding this comment.
Any chance it's possible to add unit tests for this?
| } | ||
|
|
||
| // SenderFunds retrieves the sender's spendable balance directly from the local sender manager cache | ||
| func (r *RedeemerClient) SenderFunds(sender ethcommon.Address) (*big.Int, error) { |
There was a problem hiding this comment.
Any chance it's possible to add unit tests for this?
| info, err := r.sm.GetSenderInfo(sender) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| totalReserve := new(big.Int).Set(info.Reserve.FundsRemaining) | ||
| if info.Reserve.ClaimedInCurrentRound != nil { | ||
| totalReserve.Add(totalReserve, info.Reserve.ClaimedInCurrentRound) | ||
| } | ||
|
|
||
| available := new(big.Int).Set(totalReserve) | ||
| if info.Deposit != nil { | ||
| available.Add(available, info.Deposit) | ||
| } |
There was a problem hiding this comment.
Any change it's possible to extract it to some utils and reuse? The code looks very similar to some other parts and the LocalSenderMonitor.SenderFunds() implementation.
What does this pull request do? Explain your changes. (required)
Adds a parameter which, instead of doing the full check to prevent double spending (split reserve + deposit across orch pool + subtract pending tickets), does a simple check: does the current deposit cover the ticket payout?
Specific updates (required)
How did you test each of these updates (required)
Not tested yet.
Does this pull request close any open issues?
Related to #3746 and #3744, but a PR with the proper fix (monitor avg gas price) will also be put up as a proper solution.
Checklist:
makeruns successfully./test.shpass