[WIP] APIv3 #625
[WIP] APIv3 #625
Conversation
dajohi
force-pushed
the
apiv3
branch
3 times, most recently
from
92c3ea5 to
d9eab00
Compare
There was a problem hiding this comment.
I recognize this is a WIP, and you'd likely have noted (some of) these reminders to circle back on. I was just reading the code to get a good idea of how to test this new feature out, noticed these and thought to leave comments.
PS: There's some nitpicking, please forgive. Those shouldn't belong in a WIP. There are a number of worth-checking out comments though.
| return nil, nil, 0, 0, err | ||
| } | ||
| if resp.Confirmations < 2 || resp.BlockHeight < 0 { | ||
| return nil, nil, 0, 0, errors.New("ticket does not enough confirmations") |
There was a problem hiding this comment.
| return nil, nil, 0, 0, errors.New("ticket does not enough confirmations") | |
| return nil, nil, 0, 0, errors.New("ticket does not have enough confirmations") |
| sDiff := blockHeader.SBits | ||
|
|
||
| // get fee address | ||
| feeAddress, err := spd.WalletConnection.RPCClient().GetNewAddress(ctx, "fees") |
There was a problem hiding this comment.
Fees are currently derived off a cold wallet's xpub, which I think is safer. Is this a temporary measure? Or do VSP admins now need to create a fee account in each of their voting wallets? If we're using the voting wallets for collecting fees, might want to make the fee account configurable.
There was a problem hiding this comment.
Maybe it will be an imported account? ie, importxpub "name" "xpub" name being "fee"
There was a problem hiding this comment.
yes, there will be an importxpub
| bytes FeeTx = 3; | ||
| } | ||
| message PayFeeResponse { | ||
| string Hash = 1; |
There was a problem hiding this comment.
FeeTxHash to avoid ambiguity.
| TicketHash string | ||
| CommitmentSignature string | ||
| FeeAddress string | ||
| Address string |
There was a problem hiding this comment.
CommitmentAddress. Prolly immediately before or after CommitmentSignature.
| } | ||
|
|
||
| // InsertFeeAddressVotingKey updates the fees table with a voting key for the given address. | ||
| func InsertFeeAddressVotingKey(dbMap *gorp.DbMap, address, votingKey string, voteBits uint16) error { |
There was a problem hiding this comment.
| func InsertFeeAddressVotingKey(dbMap *gorp.DbMap, address, votingKey string, voteBits uint16) error { | |
| func SetFeeAddressVotingKey(dbMap *gorp.DbMap, address, votingKey string, voteBits uint16) error { |
| func InsertFeeAddressVotingKey(dbMap *gorp.DbMap, address, votingKey string, voteBits uint16) error { | ||
| _, err := dbMap.Exec("UPDATE Fees SET VotingKey = ?, VoteBits = ? WHERE Address = ?", votingKey, voteBits, address) | ||
| if err != nil { | ||
| log.Warnf("InsertFeeAddressPrivKey: %v", err) |
| for i, conn := range s.grpcConnections { | ||
| client := pb.NewStakepooldServiceClient(conn) | ||
| responses[i], err = client.GetFeeAddress(ctx, &pb.GetFeeAddressRequest{ | ||
| Hash: txHash.String(), | ||
| Signature: signature, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("GetFeeAddress RPC failed on stakepoold instance %s: %v", conn.Target(), err) | ||
| } | ||
| } | ||
| return responses[0], nil |
There was a problem hiding this comment.
2 questions here:
- This method errors if any of the voting wallets do not return an address but the addresses returned from all backends are not compared for consistency. If it doesn't matter whether they all return the same address, I suppose it should likewise not matter if 1 or more backends do not return an address.
- If a previous call to this method ends up deriving a new address for some wallets and not others, a new call to the method would produce different addresses. I expect the wallet that failed previously would be an index behind and would not be tracking usage for the newer address. I think that just means the balance may not be accurate on that wallet, so no biggie perhaps. If no biggie, could just ignore
GetFeeAddress RPC failureon any failing backend.
I still think using a cold wallet's xpub is better anyways.
There was a problem hiding this comment.
🤔 reply meant for another comment perhaps?
| for i, conn := range s.grpcConnections { | ||
| client := pb.NewStakepooldServiceClient(conn) | ||
| responses[i], err = client.PayFee(ctx, &pb.PayFeeRequest{ | ||
| TicketHash: txHash.String(), | ||
| VotingWIF: votingWIF.String(), | ||
| FeeTx: feeTxBuffer.Bytes(), | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("GetFeeAddress RPC failed on stakepoold instance %s: %v", conn.Target(), err) | ||
| } | ||
| } | ||
| return responses[0], nil |
There was a problem hiding this comment.
2 concerns here
- Calling
PayFeeon multiple backends would cause multiple fee tx broadcast attempts. The broadcast could fail on any of those backends, no? - If
PayFeeerrors on one backend, the fee tx might have been broadcasted by another backend. Should probably ignore errors if thePayFeecall to the first backend succeeds.
|
What is the status of this PR now that vspd is announced? |
Work towards #574