Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cap-0030 - remove NO_ISSUER results #2389

Merged
merged 2 commits into from
Mar 31, 2020
Merged

cap-0030 - remove NO_ISSUER results #2389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a041b75
stop prefetching issuer
sisuresh Mar 13, 2020
c0ce78a
cap-0030 - remove NO_ISSUER results for all but changeTrustOp
sisuresh Mar 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 17 additions & 10 deletions src/transactions/ManageOfferOpFrameBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,19 @@ ManageOfferOpFrameBase::checkOfferValid(AbstractLedgerTxn& ltxOuter)
return true;
}

auto ledgerVersion = ltx.loadHeader().current().ledgerVersion;

if (mSheep.type() != ASSET_TYPE_NATIVE)
{
auto sheepLineA = loadTrustLine(ltx, getSourceID(), mSheep);
auto issuer = stellar::loadAccount(ltx, getIssuer(mSheep));
if (!issuer)
if (ledgerVersion < 13)
{
setResultSellNoIssuer();
return false;
auto issuer = stellar::loadAccount(ltx, getIssuer(mSheep));
if (!issuer)
{
setResultSellNoIssuer();
return false;
}
}
if (!sheepLineA)
{ // we don't have what we are trying to sell
Expand All @@ -67,11 +72,15 @@ ManageOfferOpFrameBase::checkOfferValid(AbstractLedgerTxn& ltxOuter)
if (mWheat.type() != ASSET_TYPE_NATIVE)
{
auto wheatLineA = loadTrustLine(ltx, getSourceID(), mWheat);
auto issuer = stellar::loadAccount(ltx, getIssuer(mWheat));
if (!issuer)

if (ledgerVersion < 13)
{
setResultBuyNoIssuer();
return false;
auto issuer = stellar::loadAccount(ltx, getIssuer(mWheat));
if (!issuer)
{
setResultBuyNoIssuer();
return false;
}
}
if (!wheatLineA)
{ // we can't hold what we are trying to buy
Expand Down Expand Up @@ -523,8 +532,6 @@ ManageOfferOpFrameBase::insertLedgerKeysToPrefetch(
auto addIssuerAndTrustline = [&](Asset const& asset) {
if (asset.type() != ASSET_TYPE_NATIVE)
{
auto issuer = getIssuer(asset);
keys.emplace(accountKey(issuer));
keys.emplace(trustlineKey(this->getSourceID(), asset));
}
};
Expand Down
16 changes: 3 additions & 13 deletions src/transactions/PathPaymentOpFrameBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,6 @@ PathPaymentOpFrameBase::insertLedgerKeysToPrefetch(
{
keys.emplace(accountKey(getDestID()));

auto processAsset = [&](Asset const& asset) {
if (asset.type() != ASSET_TYPE_NATIVE)
{
auto issuer = getIssuer(asset);
keys.emplace(accountKey(issuer));
}
};

processAsset(getSourceAsset());
processAsset(getDestAsset());
std::for_each(getPath().begin(), getPath().end(), processAsset);

if (getDestAsset().type() != ASSET_TYPE_NATIVE)
{
keys.emplace(trustlineKey(getDestID(), getDestAsset()));
Expand All @@ -53,7 +41,9 @@ PathPaymentOpFrameBase::checkIssuer(AbstractLedgerTxn& ltx, Asset const& asset)
{
if (asset.type() != ASSET_TYPE_NATIVE)
{
if (!stellar::loadAccountWithoutRecord(ltx, getIssuer(asset)))
uint32_t ledgerVersion = ltx.loadHeader().current().ledgerVersion;
if (ledgerVersion < 13 &&
!stellar::loadAccountWithoutRecord(ltx, getIssuer(asset)))
{
setResultNoIssuer(asset);
return false;
Expand Down
3 changes: 0 additions & 3 deletions src/transactions/PaymentOpFrame.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,6 @@ PaymentOpFrame::insertLedgerKeysToPrefetch(
// Prefetch issuer for non-native assets
if (mPayment.asset.type() != ASSET_TYPE_NATIVE)
{
auto issuer = getIssuer(mPayment.asset);
keys.emplace(accountKey(issuer));

// These are *maybe* needed; For now, we load everything
keys.emplace(trustlineKey(mPayment.destination, mPayment.asset));
keys.emplace(trustlineKey(getSourceID(), mPayment.asset));
Expand Down
58 changes: 42 additions & 16 deletions src/transactions/test/ManageBuyOfferTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,49 @@ TEST_CASE("manage buy offer failure modes", "[tx][offers]")

SECTION("check offer valid")
{
SECTION("sell no issuer")
uint32_t ledgerVersion;
{
auto a1 = root.create("a1", minBalance1PlusFees);
REQUIRE_THROWS_AS(
a1.manageBuyOffer(0, makeAsset(getAccount("fake"), "CUR3"),
native, Price{1, 1}, 1),
ex_MANAGE_BUY_OFFER_SELL_NO_ISSUER);
LedgerTxn ltx(app->getLedgerTxnRoot());
ledgerVersion = ltx.loadHeader().current().ledgerVersion;
}

SECTION("no issuer")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: No reason to add an extra SECTION here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you missed this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section is required because it holds code that removes the issuer, and I don't want this to affect the other tests. I believe the section was unnecessary in my initial commit, but then I moved common code into this section. I should've mentioned this in my comment instead of "Fixed".

{
auto a1 = root.create("a1", minBalance3PlusFees);
a1.changeTrust(cur1, INT64_MAX);
issuer1.pay(a1, cur1, 1);
closeLedgerOn(*app, 2, 1, 1, 2016);

// remove issuer
issuer1.merge(root);

SECTION("sell no issuer")
jonjove marked this conversation as resolved.
Show resolved Hide resolved
{
if (ledgerVersion < 13)
{
jonjove marked this conversation as resolved.
Show resolved Hide resolved
REQUIRE_THROWS_AS(
a1.manageBuyOffer(0, cur1, native, Price{1, 1}, 1),
ex_MANAGE_BUY_OFFER_SELL_NO_ISSUER);
}
else
{
a1.manageBuyOffer(0, cur1, native, Price{1, 1}, 1);
}
}

SECTION("buy no issuer")
{
if (ledgerVersion < 13)
{
REQUIRE_THROWS_AS(
a1.manageBuyOffer(0, native, cur1, Price{1, 1}, 1),
ex_MANAGE_BUY_OFFER_BUY_NO_ISSUER);
}
else
{
a1.manageBuyOffer(0, native, cur1, Price{1, 1}, 1);
}
}
}

SECTION("sell no trust")
Expand Down Expand Up @@ -185,16 +221,6 @@ TEST_CASE("manage buy offer failure modes", "[tx][offers]")
ex_MANAGE_BUY_OFFER_SELL_NOT_AUTHORIZED);
}

SECTION("buy no issuer")
{
auto a1 = root.create("a1", minBalance1PlusFees);
REQUIRE_THROWS_AS(
a1.manageBuyOffer(0, native,
makeAsset(getAccount("fake"), "CUR3"),
Price{1, 1}, 1),
ex_MANAGE_BUY_OFFER_BUY_NO_ISSUER);
}

SECTION("buy no trust")
{
auto a1 = root.create("a1", minBalance1PlusFees);
Expand Down
60 changes: 38 additions & 22 deletions src/transactions/test/OfferTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,16 +159,25 @@ TEST_CASE("create offer", "[tx][offers]")
SECTION("create offer without issuer for selling")
{
auto a1 = root.create("A", minBalance2);
auto fakeIssuer = getAccount("fakeIssuer");
for_all_versions(*app, [&] {
REQUIRE_THROWS_AS(market.requireChangesWithOffer(
{},
[&] {
return market.addOffer(
a1, {makeAsset(fakeIssuer, "IDR"),
usd, oneone, 100});
}),
ex_MANAGE_SELL_OFFER_SELL_NO_ISSUER);
a1.changeTrust(idr, trustLineLimit);
issuer.pay(a1, idr, trustLineLimit);
closeLedgerOn(*app, 2, 1, 1, 2016);
// remove issuer
issuer.merge(root);
for_versions_to(12, *app, [&] {
REQUIRE_THROWS_AS(
market.requireChangesWithOffer(
{},
[&] {
return market.addOffer(a1, {idr, xlm, oneone, 100});
}),
ex_MANAGE_SELL_OFFER_SELL_NO_ISSUER);
});

for_versions_from(13, *app, [&] {
jonjove marked this conversation as resolved.
Show resolved Hide resolved
market.requireChangesWithOffer({}, [&] {
return market.addOffer(a1, {idr, xlm, oneone, 100});
});
});
}

Expand Down Expand Up @@ -207,18 +216,25 @@ TEST_CASE("create offer", "[tx][offers]")
{
auto a1 = root.create("A", minBalance2);
a1.changeTrust(idr, trustLineLimit);
issuer.pay(a1, idr, trustLineLimit);
auto fakeIssuer = getAccount("fakeIssuer");
for_all_versions(*app, [&] {
REQUIRE_THROWS_AS(market.requireChangesWithOffer(
{},
[&] {
return market.addOffer(
a1, {idr,
makeAsset(fakeIssuer, "USD"),
oneone, 100});
}),
ex_MANAGE_SELL_OFFER_BUY_NO_ISSUER);
issuer.pay(a1, idr, 100);
closeLedgerOn(*app, 2, 1, 1, 2016);
// remove issuer
issuer.merge(root);

for_versions_to(12, *app, [&] {
REQUIRE_THROWS_AS(
market.requireChangesWithOffer(
{},
[&] {
return market.addOffer(a1, {xlm, idr, oneone, 100});
}),
ex_MANAGE_SELL_OFFER_BUY_NO_ISSUER);
});

for_versions_from(13, *app, [&] {
market.requireChangesWithOffer({}, [&] {
return market.addOffer(a1, {xlm, idr, oneone, 100});
});
});
}

Expand Down
Loading