Asset backed offers #1718

jonjove · 2018-06-25T17:01:43Z

Implements cap-0003, which has been discussed in stellar/stellar-protocol#36. Note that this is a preliminary implementation and does not yet contain all necessary tests, so it should not be merged.

src/xdr/Stellar-ledger-entries.x

@@ -123,6 +129,16 @@ struct AccountEntry
    {
    case 0:
        void;
+    case 1:


MonsieurNicolas · 2018-06-25T18:25:17Z

src/bucket/Bucket.cpp

@@ -261,7 +261,7 @@ compareSizes(std::string const& objType, uint64_t inDatabase,
 // in the ledger frames and db layer to make that work.

 void
-checkDBAgainstBuckets(medida::MetricsRegistry& metrics,
+checkDBAgainstBuckets(uint32_t ledgerVersion, medida::MetricsRegistry& metrics,


I think you should split this commit:

one for adding ledger version support when loading ledger entries

the other for adding liabilities support to account (including SQL schema upgrade)

The version support no longer exists after these fixes.

MonsieurNicolas · 2018-06-25T18:43:15Z

src/invariant/BucketListIsConsistentWithDatabase.cpp

@@ -99,7 +100,8 @@ BucketListIsConsistentWithDatabase::checkOnBucketApply(
            default:
                abort();
            }
-            auto s = EntryFrame::checkAgainstDatabase(e.liveEntry(), mDb);
+            auto s = EntryFrame::checkAgainstDatabase(
+                mLedgerManager.getCurrentLedgerVersion(), e.liveEntry(), mDb);


I think this is logically wrong: mLedgerManager.getCurrentLedgerVersion() has nothing to do with the buckets being applied ; in fact this is only set after buckets are applied (look for ProgressState::APPLIED_BUCKETS)

Removed version-based loading.

MonsieurNicolas · 2018-06-25T18:44:17Z

src/invariant/BucketListIsConsistentWithDatabaseTests.cpp

@@ -448,6 +452,7 @@ class ApplyBucketsWorkModifyEntry : public ApplyBucketsWork
        , mKey(target.first)
        , mFrame(target.second)
        , mModified{false}
+        , mLedgerVersion(app.getLedgerManager().getCurrentLedgerVersion())


similar to catchup: currentLedgerVersion doesn't seem to be a version that has any good meaning

Removed version-based loading.

MonsieurNicolas · 2018-06-25T18:55:03Z

src/ledger/AccountFrame.cpp

@@ -273,6 +282,17 @@ AccountFrame::loadAccount(AccountID const& accountID, Database& db)
                               signers.end());
    }

+    if (ledgerVersion >= 11)
+    {
+        assert(buyingLiabilitiesInd == sellingLiabilitiesInd);


I think that we should just be doing this:

assert(buyingLiabilitiesInd == sellingLiabilitiesInd); if (buyingLiabilitiesInd == soci::i_ok) { account.ext.v(1); account.ext.v1().liabilities = liabilities; }

the check as you wrote it doesn't actually enforce anything useful.
The liabilities being set transparently to 0 like this is just error prone as we throw away the information (by calling account.ext.v(1)).
The conversion from no liabilities -> with liabilities should be done 100% in the upgrade code - modifying entries on load is strange as, in theory, we're supposed to enforce that entries get written back if we do it like this (or risk having SQL and buckets get out of sync).

I think this also gets rid of the need to pass around the version number (previous commit)

Agreed, modifying on load is strange. Liabilities are now only enabled via addBuyingLiabilities and addSellingLiabilities.

src/database/Database.cpp

    case 6:
        mSession << "ALTER TABLE peers ADD flags INT NOT NULL DEFAULT 0";
        break;
+
+    case 7:
+        Upgrades::dropAll(*this);


MonsieurNicolas · 2018-06-26T01:58:48Z

src/invariant/LiabilitiesMatchOffers.cpp

+        if (shouldCheckAccount(*iter, ledgerVersion))
+        {
+            auto const& account = current.data.account();
+            Liabilities liabilities = (ledgerVersion >= 11)


instead of using ledgerVersion, you can simply use account.ext.v() == 1

This is incorrect with the new changes that liabilities should be managed only by AccountFrame and TrustFrame functions get*Liabilities and add*Liabilities. In tests, accounts and trust lines created during the set-up phase (which occurs in the current protocol version) may have liabilities. These would then be incorrectly checked here regardless of the version if this change was made.

MonsieurNicolas · 2018-06-26T01:59:26Z

src/invariant/LiabilitiesMatchOffers.cpp

+    {
+        auto const& trust = current.data.trustLine();
+        Liabilities liabilities =
+            (ledgerVersion >= 11) ? trust.ext.v1().liabilities : Liabilities{};


same here, better to use trust.ext.v()

Same response as above.

MonsieurNicolas · 2018-06-26T02:00:03Z

src/invariant/LiabilitiesMatchOffers.cpp

@@ -0,0 +1,249 @@
+// Copyright 2018 Stellar Development Foundation and contributors. Licensed


new/deleted files -> need to update visual studio project file

MonsieurNicolas · 2018-06-26T02:02:46Z

src/test/test.cpp

@@ -88,7 +88,7 @@ getTestConfig(int instanceNumber, Config::TestDbMode mode)
                                       "CacheIsConsistentWithDatabase",
                                       "ConservationOfLumens",
                                       "LedgerEntryIsValid",
-                                       "MinimumAccountBalance"};
+                                       "LiabilitiesMatchOffers"};


forgot to update the example config (documentation for invariants)

vogel · 2018-06-26T19:29:37Z

src/invariant/CacheIsConsistentWithDatabase.cpp

@@ -16,11 +17,13 @@ std::shared_ptr<Invariant>
 CacheIsConsistentWithDatabase::registerInvariant(Application& app)
 {
    return app.getInvariantManager()
-        .registerInvariant<CacheIsConsistentWithDatabase>(app.getDatabase());
+        .registerInvariant<CacheIsConsistentWithDatabase>(


To think about: is it better to pass just the objects that we need or just give up and pass whole Application everywhere (as it seems that over time more and more objects are required in multiple places)?

In general, I think it is better to pass just the objects that are needed. However, the Invariant subsystem was designed so that all invariants appear the same externally (registering and enabling every invariant should look exactly the same). That is why we pass Application into the Invariant-level registerInvariant which then forwards only the relevant subsystems to the InvariantManager-level registerInvariant.

vogel · 2018-06-26T19:30:28Z

src/invariant/CacheIsConsistentWithDatabase.cpp

-    : Invariant(false), mDb{db}
+CacheIsConsistentWithDatabase::CacheIsConsistentWithDatabase(Database& db,
+                                                             LedgerManager& lm)
+    : Invariant(false), mDb{db}, mLedgerManager(lm)


Note: false here is cryptic, should be replaced with enum when that makes sense.

I agree that this is cryptic and should be fixed, but I think it is outside the scope of this PR as it doesn't really have anything specific to do with cap-0003.

vogel · 2018-06-26T20:06:44Z

src/ledger/AccountFrame.cpp

+bool
+AccountFrame::addSellingLiabilities(int64_t delta, LedgerManager& lm)
+{
+    return stellar::addBalance(mAccountEntry.ext.v1().liabilities.selling,


I'm not sure that is valid. getBalanceAboveReserve already uses current value of getSellingLiabilities.

Lets assume current sellingliabilities = 20, balance = 60, minbalance = 20.
If we want to increase sellingliabilities by 20 to 40 (which should be possible), we call this method.

getBalanceAboveReserve then returns balance - minbalance - sellingliabiliies, which is 20. This is passes as maxBalance to the addBalance method, so we are not able to increase sellingliabilities.

Excellent catch, fixed.

vogel · 2018-06-26T20:16:23Z

src/transactions/AllowTrustOpFrame.cpp

+
+        auto offers = OfferFrame::loadOffersByAccountAndAsset(
+            mAllowTrust.trustor, ci, db);
+        for (auto& offer : offers)


or even better - extract this part to method called deletAllLinkedOffers

vogel · 2018-06-26T20:20:27Z

src/transactions/ManageOfferOpFrame.cpp

+            mSheepLineA.reset();
+
+            int64_t buyingLiabilities = mSellSheepOffer->getBuyingLiabilities();
+            Asset const& buyingAsset = mSellSheepOffer->getBuying();


this part looks very similar to change in account

is there any chance of some function extraction and code de-duplication here or is that not possible?

This segment has been refactored into a pair of functions releaseLiabilities and acquireLiabilities which facilitate liability management and is used in a few places such as ManageOfferOpFrame, AllowTrustOpFrame, and OfferExchange.

vogel · 2018-06-27T08:49:01Z

src/herder/Upgrades.cpp

+        auto accountFrame =
+            AccountFrame::loadAccount(ld, accountOffers.first, db);
+        auto& account = accountFrame->getAccount();
+        assert(accountFrame);


this makes no sense here, should be moved line up

Good catch. Previous line is actually deleted after these fixes, so done.

vogel · 2018-06-27T08:57:24Z

src/herder/Upgrades.cpp

+               AccountID const& accountID, Asset const& asset, int64_t delta)
+{
+    auto iter =
+        liabilities.insert(std::make_pair(asset, std::make_unique<int64_t>(0)))


we are probably executing make_unique too many times here, but as it is code that will be executed only once, it should no affect performance

I agree that it is used many times, but I think trying to use it less might make determining the correctness much more difficult.

vogel · 2018-06-27T09:10:24Z

src/herder/Upgrades.cpp

+}
+
+void
+Upgrades::storeUpgradeHistory(LedgerManager& ledgerManager,


could probable be split into two commits - one for storeUpgradeHistory and friends, other for prepareLiabilities

Good idea, done.

vogel · 2018-06-27T10:05:50Z

src/herder/Upgrades.cpp

+}
+
+static void
+prepareLiabilities(LedgerManager& ledgerManager, LedgerDelta& ld)


I'm not sure if I understand this method correctly, please let me know if this is correct:

For each account, remove all offers that are using assets, for which the liabilities are too big. So it does not keep any offers from that set, even if keeping only some of them would make all the liabilities proper?

You do understand this correctly, and I have added a comment to clarify. It works like this for two reasons:

I don't think there is any natural method to determine which offers should remain from a given asset

This method is independent of the order in which the offers are processed across assets

vogel · 2018-06-27T10:23:50Z

src/herder/UpgradesTests.cpp

+
+    SECTION("liabilities overflow")
+    {
+        auto createOfferSmall =


seems not to be used

Correct, it was a remnant of an earlier test. Fixed.

vogel · 2018-06-27T10:28:33Z

src/transactions/OfferTests.cpp

+                for_versions_to(11, *app, [&] {
+                    issuer.pay(a1, usd, 20000);
+
+                    // ensure we could receive proceeds from the offer


probably invalid comment

vogel · 2018-06-27T10:30:06Z

src/transactions/OfferTests.cpp

@@ -1039,6 +1170,12 @@ TEST_CASE("create offer", "[tx][offers]")
                         {b1, {{usd, 150}, {idr, trustLineBalance - 100}}},
                         {c1, {{usd, trustLineBalance - 225}, {idr, 150}}}});
                });
+
+                for_versions_from(11, *app, [&] {
+                    // makes "A" only capable of holding 75 "USD"


invalid comment

src/ledger/OfferFrame.cpp

+{
+    std::vector<OfferFrame::pointer> retOffers;
+    std::string sql = offerColumnSelector;
+    sql += " WHERE sellerid = :acc"


MonsieurNicolas · 2018-06-30T00:17:33Z

src/ledger/OfferFrame.cpp

+                                        LedgerDelta& delta, Database& db,
+                                        LedgerManager& ledgerManager)
+{
+    auto loadAccountIfNecessaryAndValidate = [this, &account, &delta, &db]() {


no need to fix this but I think that explicit captures is only useful when doing multi-threaded/async calls.

MonsieurNicolas · 2018-06-30T00:33:56Z

src/transactions/ManageOfferOpFrame.cpp

                {
                    // this would indicate a bug in OfferExchange
                    throw std::runtime_error("offer claimed over limit");
                }

-                mSourceAccount->storeChange(delta, db);
+                mSourceAccount->storeChange(tempDelta, db);


sad, thanks for catching this

MonsieurNicolas · 2018-06-30T00:39:57Z

src/transactions/TransactionFrame.cpp

@@ -352,6 +350,35 @@ TransactionFrame::commonValid(SignatureChecker& signatureChecker,
    }

    return ValidationType::kFullyValid;
+
+    //// if we are in applying mode fee was already deduced from signing account


needs to be removed

This was commented out as I was updating this to use getBalanceAboveReserve. Fixed.

MonsieurNicolas · 2018-06-30T00:41:18Z

docs/db-schema.md

+
+## upgradehistory
+
+Defined in [`src/transactions/TransactionFrame.cpp`](/src/herder/Upgrades.cpp)


MonsieurNicolas · 2018-07-01T19:09:11Z

src/transactions/OfferTests.cpp

@@ -572,6 +590,17 @@ TEST_CASE("create offer", "[tx][offers]")
                        auto actualPayment = a1IDrs - delta;
                        checkCrossed(b1, actualPayment, offerAmount);
                    });
+
+                    for_versions_from(10, *app, [&]() {
+                        REQUIRE_THROWS_AS(


this is not what this test is trying to test:
test ensures that we compute reserve as if creating the offer.
I think that, with liabilities, there should be both a check that verifies that creating an offer with exactly actualPayment works, but trying to sell more (by 1 stroop) fails.

MonsieurNicolas · 2018-07-01T19:17:24Z

src/transactions/OfferTests.cpp

+
+                for_versions_from(10, *app, [&]() {
+                    // make it that c1 can only receive 9 USD (for 1 IDR)
+                    REQUIRE_THROWS_AS(c1.changeTrust(usd, 9 * assetMultiplier),


same thing here: the way the setup is done is now impossible, probably clearer to not test this anymore like that and instead rely on changeTrust tests

MonsieurNicolas · 2018-07-01T19:22:21Z

src/transactions/OfferTests.cpp

+            });
+            REQUIRE_THROWS_AS(market.updateOffer(a1, offer.key.offerID,
+                                                 {xlm, usd, oneone, 111},
+                                                 {xlm, usd, oneone, 110}),


it's a bit confusing to have an expected state different as it's a failure case

Agreed, fixed.

MonsieurNicolas · 2018-07-01T19:25:23Z

src/transactions/OfferTests.cpp

@@ -1292,6 +1431,15 @@ TEST_CASE("create offer", "[tx][offers]")
                                          {xlm, usd, oneone, 110});
            });
        });
+        for_versions_from(10, *app, [&] {
+            auto offer = market.requireChangesWithOffer({}, [&] {
+                return market.addOffer(a1, {xlm, usd, oneone, 9});


I think we should change the amounts to be the ones around the edge case:
so use "10" instead of "9", and use "11" instead of "111"

MonsieurNicolas · 2018-07-01T19:37:18Z

src/transactions/PathPaymentTests.cpp

-                 {mm34, {{xlm, minBalance3 - 3 * txfee}, {cur1, 0}, {cur2, 0}, {cur3, 20}, {cur4, 0}}},
-                 {destination, {{xlm, minBalance1 - txfee}, {cur1, 0}, {cur2, 0}, {cur3, 0}, {cur4, 10}}}});
-            // clang-format on
+            REQUIRE_THROWS_AS(mm12a.pay(gateway, cur2, 39),


should those edge cases be tested differently?

These edge cases aren't possible anymore, since it is not possible to have an offer with excess liabilities. Let's not test them starting in version 10.

marta-lokhova · 2018-06-27T22:19:53Z

src/herder/UpgradesTests.cpp

+    VirtualClock clock;
+    auto cfg = getTestConfig(0);
+    cfg.LEDGER_PROTOCOL_VERSION = 9;
+    auto app = Application::create(clock, cfg);


would be better to use createTestApplication instead (same for line 1011).

Good idea. All uses of Application::create in this file changed to createTestApplication.

marta-lokhova · 2018-07-03T00:14:46Z

src/ledger/AccountFrame.cpp

+int64_t
+getBuyingLiabilities(AccountEntry const& acc, LedgerManager const& lm)
+{
+    assert(lm.getCurrentLedgerVersion() >= 10);


You're comparing to version 10 is many places, any reason not to have it as a constant?

We haven't followed this convention with other version numbers, and I'm not sure it would add much clarity. For example, do you think assert(lm.getCurrentLedgerVersion() >= PROTOCOL_VERSION_10); would be considerably more clear?

The reasons I suggested it:

You can easily change it. Previously you had 11, so instead of changing it to 10 in every place, you could just change one variable.

you could name it something like PROTOCOL_VERSION_WITH_LIABILITIES and it would be way less vague than just 10

let's not do that in this PR: v10 is broader than liabilities.

It could even to as far as if (lm.supportsLiabilities()) { ... }

yeah like I said, not for this PR; and this would make the code even more complicated to understand as features depend on each other on a per protocol basis.

MonsieurNicolas · 2018-07-06T01:42:58Z

src/transactions/ManageDataTests.cpp

+            acc1.manageData(t1, &value);
+        });
+        for_versions_from(10, *app, [&] {
+            REQUIRE_THROWS_AS(acc1.manageData(t1, &value),


nice, I was thinking that we need to cover this in general

MonsieurNicolas · 2018-07-06T19:03:46Z

src/transactions/PaymentTests.cpp

+            auto const minBal0 = app->getLedgerManager().getMinBalance(0);
+            auto const minBal3 = app->getLedgerManager().getMinBalance(3);
+
+            auto txfee = app->getLedgerManager().getTxFee();


txfee: this is redefining/shadowing line 43 (can be removed)

MonsieurNicolas · 2018-07-06T19:05:13Z

src/ledger/LedgerManagerImpl.cpp

+            // Note: Index from 1 rather than 0 to match the behavior of
+            // storeTransaction and storeTransactionFee.
+            Upgrades::storeUpgradeHistory(*this, lupgrade,
+                                          upgradeDelta.getChanges(), i + 1);


i+1 is of type size_t - yet storeUpgradeHistory takes an int, so this triggers a warning

MonsieurNicolas · 2018-07-06T22:26:25Z

src/ledger/LedgerManagerImpl.cpp

+
+    // It is required to rollback the current LedgerHeader in order to satisfy
+    // the consistency checks enforced by LedgerDelta::commit.
+    getCurrentLedgerHeader() = initialHeader;


I think you mean previousHeader here as to only rollbck whatever upgrade did?

vogel · 2018-07-09T11:19:57Z

src/ledger/AccountFrame.h

@@ -23,6 +23,9 @@ namespace stellar
 class LedgerManager;
 class LedgerRange;

+int64_t getBuyingLiabilities(AccountEntry const& acc, LedgerManager const& lm);


Looks like versions of those for AccountEntry and TrustLineEntry could be merged as a template.

vogel · 2018-07-09T11:22:45Z

src/ledger/OfferFrame.cpp

+    };
+
+    auto loadTrustIfNecessaryAndValidate = [this, &delta, &db](
+                                               TrustFrame::pointer const& trust,


missing clang-format?

This PR is only partially rebased right now, so no clang-format on the newest commits yet.

vogel · 2018-07-09T11:24:38Z

src/ledger/OfferFrame.cpp

+            acc = AccountFrame::loadAccount(delta, getSellerID(), db);
+            assert(acc);
+        }
+        assert(acc->getID() == getSellerID());


I do not see need for such asserts here. If we wish to ensure that loadAccount does what it says it does we could either:

test it with some unit tests

make that assert a postcondition on loadAccount inside that function

This assert was actually put here to guarantee that if the parameter account != nullptr then that account matches the account that owns this offer. I agree that it does not need to test the case for AccountFrame::loadAccount. But given that I already wanted to test this condition for one case, I did not see a big disadvantage in checking all cases.

Right, I have missed that case.

vogel · 2018-07-09T11:42:19Z

src/transactions/InflationOpFrame.cpp

@@ -108,7 +108,7 @@ InflationOpFrame::doApply(Application& app, LedgerDelta& delta,
            {
                lcl.totalCoins += toDoleThisWinner;
            }
-            if (!winner->addBalance(toDoleThisWinner))
+            if (!winner->addBalance(toDoleThisWinner, ledgerManager))
            {
                throw std::runtime_error(


This throw was added here as it was impossible for above condition even to get true.

But is it impossible now? Imagine an issuer that creates asset that requires authorization. It sends INT64_MAX of that asset to one account that has enough XLM to be a winner in inflation. Then that account can make an offer to sell this asset 1:1 for XLM, making its buying liabilities so big, that its addBalance returns 0. Then at the next inflation round all nodes will drop this transaction with txINTERNAL_ERROR, making inflation operations no-op for as long as they want.

I think we need another solution for handling that case now.

Good catch @vogel : I think you are correct, toDoleThisWinner should be capped by the amount that the account can receive instead of just being bigDivide(amountToDole, w.mVotes, totalVotes, ROUND_DOWN).

Good catch indeed. Fixed as described above.

vogel · 2018-07-09T11:48:28Z

src/transactions/ManageOfferOpFrame.cpp

+
+            int64_t availableBalance =
+                (sheep.type() == ASSET_TYPE_NATIVE)
+                    ? mSourceAccount->getBalanceAboveReserve(ledgerManager)


I think we could change getBalanceAboveReserver name to getAvailableBalance for symmetry?

Good idea, done.

vogel · 2018-07-09T12:27:45Z

src/herder/Upgrades.cpp

+    ledgerManager.getCurrentLedgerHeader().baseReserve = newReserve;
+    if (header.ledgerVersion >= 10 && didReserveIncrease)
+    {
+        prepareLiabilities(ledgerManager, ld);


Looks like it is possible to execute prepareLiabilities twice during closing one ledger. Maybe applyUpgrade could return bool/enum/set to show that prepareLiabilities needs to be performed, then it would be only called once in closeLedger?

no, this implementation is better as it associates upgrade side effects to the appropriate upgrade.

vogel · 2018-07-09T20:20:49Z

src/transactions/OfferTests.cpp

+                // Test when no existing offers
+                root.pay(acc1, xlm, 500 + txfee);
+                REQUIRE_THROWS_AS(
+                    market.addOffer(acc1, {xlm, usd, Price{1, 1}, 501},


I think that addOffer API allows you to leave last parameter if it will be the same as second one, as it defaults to OfferState::SAME

Good point, fixed on many lines.

vogel · 2018-07-09T20:24:41Z

src/transactions/TxResultsTests.cpp

+
+    SECTION("fees with liabilities")
+    {
+        auto acc = root.create("acc", lm.getMinBalance(1) + baseFee + 1000);


I think that edge case here is that if you added +1 to that balance, it would pass even for version 1. Maybe it is worth adding such test case for contrast?

bartekn · 2018-07-10T13:31:53Z

src/herder/Upgrades.cpp

+// using the initial result of step (1), so it does not matter what order the
+// offers are processed.
+static void
+prepareLiabilities(LedgerManager& ledgerManager, LedgerDelta& ld)


Shouldn't this function also remove offers from accounts with trustline.isAuthorized() == false? AllowTrust operation removes them when revoking trust so it seems that leaving them out is an invalid state.

prepareLiabilities does remove offers that are not authorized, as demonstrated in https://github.com/stellar/stellar-core/pull/1718/files#diff-64c71f9133aa77f79d1d123bc6a59cf0R1086. The implementation uses the following logic:

On lines 426-434, getAvailableBalance returns 0 if not authorized.

On lines 453-461, getAvailableLimit returns 0 if not authorized.

On line 542, erase == true if offerFrame.getSellingLiabilities() == 0 || offerFrame.getBuyingLiabilities() == 0.

On lines 544-550, these checks can only be reached if erase == false after line 542. If so, we check that the liabilities do not exceed the relevant cap (either available balance or available limit, depending on whether it is a buying asset or a selling asset for the offer). If either the buying or selling asset is not authorized, then the cap will be 0. But because erase == false, the liabilities cannot be 0 so we end up with erase == true.

bartekn · 2018-07-10T15:47:21Z

src/herder/Upgrades.cpp

+                                                            asset, db, &ld);
+                int64_t deltaSelling =
+                    liab.selling -
+                    trustFrame->getSellingLiabilities(ledgerManager);


This function errors when there are orphaned offers:

Exception during upgrade: trustline does not exist

Repro:

Run standalone testnet (testnet passphrase).

Submit:

AAAAAGL8HQvQkbK2HA3WVjRrKmjX00fG8sLI7m0ERwJW/AX3AAABkAAAAAAAAAABAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAAADJNEgW9b6gnIKGtYeSq1SiCyMJLujoFBoDtnljGf65oAAAAXSHboAAAAAAEAAAAADJNEgW9b6gnIKGtYeSq1SiCyMJLujoFBoDtnljGf65oAAAAGAAAAAVVTRAAAAAAAYvwdC9CRsrYcDdZWNGsqaNfTR8bywsjubQRHAlb8BfcAAAAAO5rKAAAAAAEAAAAADJNEgW9b6gnIKGtYeSq1SiCyMJLujoFBoDtnljGf65oAAAADAAAAAAAAAAFVU0QAAAAAAGL8HQvQkbK2HA3WVjRrKmjX00fG8sLI7m0ERwJW/AX3AAAAAACYloAAAAABAAAAAQAAAAAAAAAAAAAAAQAAAAAMk0SBb1vqCcgoa1h5KrVKILIwku6OgUGgO2eWMZ/rmgAAAAYAAAABVVNEAAAAAABi/B0L0JGythwN1lY0aypo19NHxvLCyO5tBEcCVvwF9wAAAAAAAAAAAAAAAAAAAAJW/AX3AAAAQK6ngzT5yLjXBKPibogU0NBm9M+W+ROt2JcPiIe1mf9eGNGaWhj0DYyG3GkTzrN4Tzg+LpXhiEO4cu6gylK6bQkxn+uaAAAAQAADEbTeCGHZ3RQVmUOX2pM8J+lFr5cq1PjOQZtoxSy1jYTnrvoRHoc7z9eSj9tv8aU/9ufEYBUYawc/l6hLMAI=

Upgrade to V10.

This was fixed in d8f0241.

MonsieurNicolas

minor nit (that you may decide to ignore). also, you need to rebase and reorder commits in commit order. If you decide to keep some of the commits as is (with fixes from previous commits), try to include the updated test cases with the commit (or have the next commit deal with the test cases).

MonsieurNicolas · 2018-07-17T20:08:29Z

src/ledger/LiabilitiesTests.cpp

@@ -505,7 +513,7 @@ TEST_CASE("liabilities", "[ledger][liabilities]")
            REQUIRE(tf->getSellingLiabilities(lm) == 0);
            if (res)
            {
-                REQUIRE(tf->getTrustLine().ext.v() == 1);
+                REQUIRE(tf->getTrustLine().ext.v() == (deltaLiabilities != 0));


while this is correct, I find the implicit conversion from true to 1 quite obscure when used like this.

vogel · 2018-07-19T09:18:48Z

src/ledger/AccountFrame.cpp

 {
-    return stellar::addBalance(mAccountEntry.balance, delta);
+    if (delta == 0 && lm.getCurrentLedgerVersion() >= 10)


why is currentLedgerVersion checked here?

Good question, I see now that it doesn't need to be checked. Fixed.

vogel · 2018-07-20T15:14:14Z

LGTM, I'll merge when acceptance tests are working

…ality of the (now-removed) MinimumAccountBalance invariant

…rade

…ions

vogel · 2018-08-01T16:09:31Z

r+ 3c9dfc0

Asset backed offers Reviewed-by: vogel

vogel · 2018-08-01T18:45:02Z

@latobarita: retry

vogel · 2018-08-02T08:26:50Z

@latobarita: retry

Asset backed offers Reviewed-by: vogel

jonjove requested a review from MonsieurNicolas June 25, 2018 17:01

MonsieurNicolas reviewed Jun 26, 2018

View reviewed changes

vogel reviewed Jun 26, 2018

View reviewed changes

vogel reviewed Jun 27, 2018

View reviewed changes

jonjove force-pushed the asset-backed-offers branch from a90176d to 66f931e Compare June 29, 2018 18:20

MonsieurNicolas reviewed Jul 1, 2018

View reviewed changes

marta-lokhova reviewed Jul 3, 2018

View reviewed changes

MonsieurNicolas reviewed Jul 6, 2018

View reviewed changes

MonsieurNicolas mentioned this pull request Jul 6, 2018

Update docs with changes to order book behavior in protocol v10 stellar-deprecated/docs#375

Closed

2 tasks

shawnfarris5150 approved these changes Jul 7, 2018

View reviewed changes

vogel reviewed Jul 9, 2018

View reviewed changes

bartekn mentioned this pull request Jul 9, 2018

Add offer liabilities stellar/go#531

Merged

1 task

vogel reviewed Jul 9, 2018

View reviewed changes

bartekn reviewed Jul 10, 2018

View reviewed changes

MonsieurNicolas reviewed Jul 17, 2018

View reviewed changes

jonjove force-pushed the asset-backed-offers branch from eeaa130 to cb2f0d6 Compare July 18, 2018 17:24

vogel reviewed Jul 19, 2018

View reviewed changes

jonjove force-pushed the asset-backed-offers branch from f4760a3 to 6093d96 Compare July 19, 2018 19:32

jonjove added 3 commits July 26, 2018 09:49

Updated XDR: added Liabilities, extended AccountEntry and TrustLineEntry

528143e

Updated AccountFrame SQL

0e8da7a

Updated TrustFrame SQL

b550642

jonjove added 10 commits July 26, 2018 09:49

Updated OfferFrame SQL

777bb87

Refactored implementation of exchangeV10 into two functions

b21170e

Updated TxSetFrame, TransactionFrame, and transactions

cdbe539

Added upgradehistory table to support upgrade-level history

1d77de8

Upgrade protocol version and base reserve maintain liabilities

92d2f48

Added LiabilitiesMatchOffers invariant which encompasses the function…

8b67e6b

…ality of the (now-removed) MinimumAccountBalance invariant

Added new transactions tests

97e5228

Added UpgradesTests for protocol version upgrade and base reserve upg…

93b16f9

…rade

Added LiabilitiesTests for balance and liability adding/getting funct…

bf5ee31

…ions

Added tests for LiabilitiesMatchOffers invariant

3c9dfc0

jonjove force-pushed the asset-backed-offers branch from adc0b0a to 3c9dfc0 Compare July 26, 2018 14:52

bartekn mentioned this pull request Jul 30, 2018

Offers in Stellar orderbooks do not use a reserve model stellar/go#389

Closed

latobarita added a commit that referenced this pull request Aug 1, 2018

Merge pull request #1718 from jonjove/asset-backed-offers

e6e2247

Asset backed offers Reviewed-by: vogel

latobarita added a commit that referenced this pull request Aug 2, 2018

Merge pull request #1718 from jonjove/asset-backed-offers

4dab962

Asset backed offers Reviewed-by: vogel

latobarita merged commit 3c9dfc0 into stellar:master Aug 2, 2018

bartekn mentioned this pull request Aug 9, 2018

Cosigned assets: NopOp and COAUTHORIZED_FLAG stellar/stellar-protocol#146

Closed

@@ @@ -123,6 +129,16 @@ struct AccountEntry @@
                   {
                   case 0:
                       void;
+                  case 1:

		@@ -0,0 +1,249 @@
		// Copyright 2018 Stellar Development Foundation and contributors. Licensed


		## upgradehistory

		Defined in [`src/transactions/TransactionFrame.cpp`](/src/herder/Upgrades.cpp)

Asset backed offers #1718

Asset backed offers #1718

Conversation

jonjove commented Jun 25, 2018

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marta-lokhova Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marta-lokhova Jul 3, 2018 •

edited

Loading