Fee-Bump Transactions #2258
Fee-Bump Transactions #2258
Conversation
| switch (txenv.type()) | ||
| { | ||
| case ENVELOPE_TYPE_TX_V0: | ||
| signtxn(txenv.v0(), sk, sha256(netId)); |
| struct FeeBumpTransaction | ||
| { | ||
| AccountID feeSource; | ||
| uint64 fee; |
There was a problem hiding this comment.
CAP update: int64 (uint64 is usually problematic)
| * a TransactionSignaturePayload */ | ||
| DecoratedSignature signatures<20>; | ||
| union TransactionEnvelope switch (EnvelopeType type) { | ||
| case ENVELOPE_TYPE_TX_V0: |
There was a problem hiding this comment.
open question:
should we do what CAP19 describes, introducing ENVELOPE_TYPE_TX.
As we have the notion of normalization (for hashing), we could mutate the payload any way we want when un-marshaling, in particular, we could just always convert to ENVELOPE_TYPE_TX and never deal with tx0.
Less code change related to the removal of the "AccountID" member (not just in this commit but in SDKs as well).
| @@ -792,6 +835,7 @@ default: | |||
|
|
|||
| enum TransactionResultCode | |||
| { | |||
| txFEE_BUMPED = 1, | |||
There was a problem hiding this comment.
need a comment here that explains that InnerTransactionResult needs to be kept in sync
| case txSUCCESS: | ||
| case txFAILED: | ||
| OperationResult results<>; | ||
| case txTOO_EARLY: |
There was a problem hiding this comment.
comment txFEE_BUMPED not included
| // under the Apache License, Version 2.0. See the COPYING file at the root | ||
| // of this distribution or at http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| #include "herder/TransactionQueue.h" |
There was a problem hiding this comment.
do not call this file FeeBumpTests.cpp : it's the same filename used in the transactions test suite
| // seqNum is unconstrained user input. | ||
| int64_t seqNum = tx->getSeqNum(); | ||
| int64_t start = transactions.front().front()->getSeqNum(); | ||
| if (seqNum >= start && (seqNum - start) < transactions.size()) |
There was a problem hiding this comment.
signed/unsigned comparison
| } | ||
| else | ||
| { | ||
| transactions.emplace_back(1, tx); |
There was a problem hiding this comment.
maybe consider using
transactions.emplace_back(std::vector<TransactionFrameBasePtr>{tx});
(easier to read than transactions.emplace_back(1, tx);)
| if ((*txIt)->getFullHash() != tx->getFullHash()) | ||
| addFee(tx); | ||
|
|
||
| auto& transactions = mPendingTransactions[tx->getSourceID()].mTransactions; |
There was a problem hiding this comment.
It seems we should have transactions be a *parameter (avoids the extra findTx call), and when null, would add the entry for that user
| @@ -542,6 +542,12 @@ CommandHandler::tx(std::string const& params, std::string& retStr) | |||
| decoder::decode_b64(blob, binBlob); | |||
|
|
|||
| xdr::xdr_from_opaque(binBlob, envelope); | |||
| if (envelope.type() == ENVELOPE_TYPE_TX_V0) | |||
There was a problem hiding this comment.
in general, it seems we're missing tests for txNOT_NORMALIZED.
Envelope test in particular: I am surprised this didn't trip in the multi sig case; maybe we should randomize them better?
We may want to land #2129 before.
|
closing as this is obsolete now |
Implements fee-bump transactions, although not exactly as proposed in https://github.com/stellar/stellar-protocol/blob/master/core/cap-0015.md. Some key changes:
TransactionEnvelopeinto an XDR unionTransactionFrameTransactionEnvelopesignatures are sorted(sourceAccount, seqNum)inTransactionQueueTxSetFrametrimming, validity checking, and surge pricingChecklist
clang-formatv5.0.0 (viamake formator the Visual Studio extension)