cap-0030 - remove NO_ISSUER results #2389
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonjove
suggested changes
Jan 24, 2020
sisuresh
force-pushed
the
removeNoIssuerResults
branch
from
402348b to
526d2ae
Compare
jonjove
reviewed
Feb 3, 2020
| a1.manageBuyOffer(0, makeAsset(getAccount("fake"), "CUR3"), | ||
| native, Price{1, 1}, 1), | ||
| ex_MANAGE_BUY_OFFER_SELL_NO_ISSUER); | ||
| SECTION("no issuer") |
There was a problem hiding this comment.
I think you missed this one.
sisuresh
force-pushed
the
removeNoIssuerResults
branch
2 times, most recently
from
adb3098 to
d66b44a
Compare
jonjove
reviewed
Mar 4, 2020
MonsieurNicolas
requested changes
Mar 11, 2020
There was a problem hiding this comment.
Aside from the lambda question in test code that @jonjove has, this looks good.
I have one suggestion to simplify the actual code changes (remove condition on prefetching)
src/transactions/PaymentOpFrame.cpp
Outdated
| { | ||
| keys.emplace(accountKey(mPayment.destination)); | ||
|
|
||
| // Prefetch issuer for non-native assets | ||
| if (mPayment.asset.type() != ASSET_TYPE_NATIVE) | ||
| { | ||
| auto issuer = getIssuer(mPayment.asset); | ||
| keys.emplace(accountKey(issuer)); | ||
| if (ledgerVersion < 13) |
There was a problem hiding this comment.
we should just stop pre-fetching issuers even for protocol 12: this avoids the code bloat related to passing ledgerVersion through the various layers
sisuresh
force-pushed
the
removeNoIssuerResults
branch
2 times, most recently
from
71b439f to
42900bc
Compare
6 tasks
sisuresh
force-pushed
the
removeNoIssuerResults
branch
from
42900bc to
c0ce78a
Compare
|
There is one comment that hasn't been addressed: #2389 (comment). Other than that, this is ready to be merged. |
|
r+ c0ce78a |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Implements https://github.com/stellar/stellar-protocol/blob/master/core/cap-0030.md
Checklist
clang-formatv5.0.0 (viamake formator the Visual Studio extension)