-
Notifications
You must be signed in to change notification settings - Fork 53
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.
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
feat: Implement MPT changes #1147
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found some things to improve 👍
Another thing is that your test coverage is slightly negative - please take a look at codecov for your branch to see where it's lacking tests. Anything with red change%
needs a bit more love.
src/rpc/handlers/MPTHolders.cpp
Outdated
|
||
mptJson[JS(account)] = toBase58(sle[ripple::sfAccount]); | ||
mptJson[JS(flags)] = sle.getFlags(); | ||
mptJson["mpt_amount"] = toBoostJson(ripple::STUInt64{sle[ripple::sfMPTAmount]}.getJson(JsonOptions::none)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no JS for these atm. i assume. Leaving it here for when they are added - please update here and below 👍
@godexsoft as for test coverage, i believe most of the parts that lack coverage are ETL related |
boost::asio::yield_context yield | ||
) const override | ||
{ | ||
auto const holderEntries = executor_.read( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually , Only this part needs to be virtual. The filter logic is same for different DB. By reducing the scope of virtual functions, you can unittest more of the logic without real DB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you clarify what you mean? inside this function, there is another call to the database auto mptObjects = doFetchLedgerObjects(mptKeys, ledgerSequence, yield);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we create a function in CassandraBackend.hpp to just select MPT holder, then all the filter logic can move to BackendInterface. If we want to add a new database, you don't need to copy all the logic to new database implementation. How to filter the valid holder is database unrelated. Instead, new database only needs to provide how to select MPT holder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But aren't existing functions in CassandraBackend.hpp also do something similar? BTW fetchMPTHolders
doesn't only have filter logic, it does make another call to the objects
table on line 576:
auto mptObjects = doFetchLedgerObjects(mptKeys, ledgerSequence, yield);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood the fetchMPTHolders
does not only have filter logic. So I suggested that we can add the pure virtual function doFetchMPTHolders
in BackendInterface, which only obtains the data from selectMPTHolders
table. Then the filer logic can move to BackendInterface normal function.
override doFetchfetchMPTHolders in CassandraBackend.hpp
std::pair<std::vector<ripple::uint256>, std::optional<ripple::AccountID>>
doFetchfetchMPTHolders(
ripple::uint192 const& mptID,
std::uint32_t const limit,
std::optional<ripple::AccountID> const& cursorIn,
boost::asio::yield_context yield
) const override
{
auto const holderEntries = executor_.read(
yield, schema_->selectMPTHolders, mptID, cursorIn.value_or(ripple::AccountID(0)), Limit{limit}
);
auto const& holderResults = holderEntries.value();
if (not holderResults.hasRows()) {
LOG(log_.debug()) << "No rows returned";
return {};
}
std::vector<ripple::uint256> mptKeys;
std::optional<ripple::AccountID> cursor;
for (auto const [holder] : extract<ripple::AccountID>(holderResults)) {
mptKeys.push_back(ripple::keylet::mptoken(mptID, holder).key);
cursor = holder;
}
return {mptKeys, cursor};
}
Then your fetchMPTHolders
will be like:
MPTHoldersAndCursor
BackendInterface::fetchMPTHolders(
ripple::uint192 const& mptID,
std::uint32_t const limit,
std::optional<ripple::AccountID> const& cursorIn,
std::uint32_t const ledgerSequence,
boost::asio::yield_context yield
) const
{
auto [mptKeys, cursor] = doFetchfetchMPTHolders(mptID, limit, cursorIn, yield);
auto mptObjects = doFetchLedgerObjects(mptKeys, ledgerSequence, yield);
auto it = std::remove_if(mptObjects.begin(), mptObjects.end(), [](Blob const& mpt) { return mpt.empty(); });
mptObjects.erase(it, mptObjects.end());
ASSERT(mptKeys.size() <= limit, "Number of keys can't exceed the limit");
if (mptKeys.size() == limit)
return {mptObjects, cursor};
return {mptObjects, {}};
}
I admit that current CassandraBackend.hpp has similar implementation which is not correct. We will refactor them eventually. Feel free to leave it. We can refactor it in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 All comments are addressed. Looks like ready to go. 👍
@@ -1 +1 @@ | |||
find_package(xrpl REQUIRED CONFIG) | |||
find_package(xrpl-mpt REQUIRED CONFIG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be reverted before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes these will be changed when it's ready to merge. We'd need wait for the rippled one to merge first
Spec is here https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0033d-multi-purpose-tokens
Builds rippled on https://github.com/gregtatcam/rippled/tree/mpt-direct-pay
Issue
andSTAmount
related changes (will need to change if rippled has further changes)mp_token_holders
mp_token_holders
table whenever a newMPTokenAuthorize
tx is observedmpt_holders
APImpt_issuance_id
field intoMPTokenIssuanceCreate
transaction metadata for APIs (eg,tx
,ledger
,subscribe
,account_tx
)MPTokenIssuance
andMPToken
objects types forledger_entry
mpt_issuance_id
field intoMPTokenIssuance
object forledger_data
andaccount_objects
APIs