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

Add MPTIssue to STIssue #5200

Merged
merged 13 commits into from
Dec 16, 2024
Merged

Add MPTIssue to STIssue #5200

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e0ffb63
Add MPTIssue to STIssue
gregtatcam Nov 22, 2024
8f3bbc4
Don't need to check/throw if not Issue. It's already done in get<>().
gregtatcam Nov 22, 2024
65533c3
Merge branch 'develop' into feature/mpt-stissue
gregtatcam Dec 5, 2024
6544945
Merge branch 'develop' into feature/mpt-stissue
gregtatcam Dec 8, 2024
ea1ecc4
Address reviewer's feedback.
gregtatcam Dec 10, 2024
4402951
Address reviewer's feedback.
gregtatcam Dec 10, 2024
cc4a9c0
Address reviewer's feedback.
gregtatcam Dec 10, 2024
10ffdf1
Address reviewer's feedback
gregtatcam Dec 11, 2024
cca7133
Apply suggestions from code review
gregtatcam Dec 11, 2024
1ae94a8
Address reviewer's feedback
gregtatcam Dec 11, 2024
0e8c062
Address reviewer's feedback - add STIssue_test unit-test
gregtatcam Dec 11, 2024
97f6d8a
Address reviewer's feedback.
gregtatcam Dec 12, 2024
166d638
Merge branch 'develop' into feature/mpt-stissue
ximinez Dec 16, 2024
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
25 changes: 25 additions & 0 deletions include/xrpl/protocol/Asset.h
View file
Open in desktop
Bronek marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,17 @@

namespace ripple {

class Asset;

template <typename TIss>
concept ValidIssueType =
std::is_same_v<TIss, Issue> || std::is_same_v<TIss, MPTIssue>;

template <typename A>
concept AssetType =
std::is_convertible_v<A, Asset> || std::is_convertible_v<A, Issue> ||
std::is_convertible_v<A, MPTIssue> || std::is_convertible_v<A, MPTID>;
Bronek marked this conversation as resolved.
Show resolved Hide resolved

/* Asset is an abstraction of three different issue types: XRP, IOU, MPT.
* For historical reasons, two issue types XRP and IOU are wrapped in Issue
* type. Many functions and classes there were first written for Issue
Expand Down Expand Up @@ -95,6 +102,9 @@ class Asset
friend constexpr bool
operator!=(Asset const& lhs, Asset const& rhs);

friend constexpr bool
operator<(Asset const& lhs, Asset const& rhs);

friend constexpr bool
operator==(Currency const& lhs, Asset const& rhs);

Expand Down Expand Up @@ -157,6 +167,21 @@ operator!=(Asset const& lhs, Asset const& rhs)
return !(lhs == rhs);
}

constexpr bool
operator<(Asset const& lhs, Asset const& rhs)
Copy link
Collaborator

@Bronek Bronek Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can you pls replace this with operator<=> ? It could look like this

    friend constexpr auto
    operator<=>(Asset const& lhs, Asset const& rhs)
    {
        return std::visit(
            [&]<typename TLhs, typename TRhs>(
                TLhs const& issLhs, TRhs const& issRhs) -> std::weak_ordering {
                if constexpr (std::is_same_v<TLhs, TRhs>)
                    return issLhs <=> issRhs;
                else
                    return lhs.issue_.index() <=> rhs.issue_.index();
            },
            lhs.issue_,
            rhs.issue_);
    }

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great if we could give std::strong_ordering to operator<=>(Issue const& lhs, Issue const& rhs) but I wouldn't want that change in this PR even if it is correct one (and I am not assuming that it is)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, will do. I actually added it in MPT-V2 (still a long way from release).

{
return std::visit(
[&]<typename TLhs, typename TRhs>(
TLhs const& issLhs, TRhs const& issRhs) {
if constexpr (std::is_same_v<TLhs, TRhs>)
return issLhs < issRhs;
else
return false;
},
Copy link
Collaborator

@Bronek Bronek Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is correct as long as we have exactly these two types i.e. Issue, MPTIssue inside issue_. As soon as that changes it will be no longer correct, meaning that this code is brittle. This is why I would prefer index comparison instead.

lhs.issue_,
rhs.issue_);
}

constexpr bool
operator==(Currency const& lhs, Asset const& rhs)
{
Expand Down
2 changes: 1 addition & 1 deletion include/xrpl/protocol/Indexes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ nft_sells(uint256 const& id) noexcept;

/** AMM entry */
Keylet
amm(Issue const& issue1, Issue const& issue2) noexcept;
amm(Asset const& issue1, Asset const& issue2) noexcept;

Keylet
amm(uint256 const& amm) noexcept;
Expand Down
19 changes: 2 additions & 17 deletions include/xrpl/protocol/MPTIssue.h
View file
Open in desktop
Bronek marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,8 @@ class MPTIssue
void
setJson(Json::Value& jv) const;

friend constexpr bool
operator==(MPTIssue const& lhs, MPTIssue const& rhs);

friend constexpr bool
operator!=(MPTIssue const& lhs, MPTIssue const& rhs);
auto
operator<=>(MPTIssue const&) const = default;
Bronek marked this conversation as resolved.
Show resolved Hide resolved

bool
native() const
Expand All @@ -64,18 +61,6 @@ class MPTIssue
}
};

constexpr bool
operator==(MPTIssue const& lhs, MPTIssue const& rhs)
{
return lhs.mptID_ == rhs.mptID_;
}

constexpr bool
operator!=(MPTIssue const& lhs, MPTIssue const& rhs)
{
return !(lhs == rhs);
}

/** MPT is a non-native token.
*/
inline bool
Expand Down
12 changes: 7 additions & 5 deletions include/xrpl/protocol/SOTemplate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ enum SOEStyle {
};

/** Amount fields that can support MPT */
enum SOETxMPTAmount { soeMPTNone, soeMPTSupported, soeMPTNotSupported };
enum SOETxMPTIssue { soeMPTNone, soeMPTSupported, soeMPTNotSupported };

//------------------------------------------------------------------------------

Expand All @@ -50,7 +50,7 @@ class SOElement
// Use std::reference_wrapper so SOElement can be stored in a std::vector.
std::reference_wrapper<SField const> sField_;
SOEStyle style_;
SOETxMPTAmount supportMpt_ = soeMPTNone;
SOETxMPTIssue supportMpt_ = soeMPTNone;

private:
void
Expand All @@ -72,10 +72,12 @@ class SOElement
{
init(fieldName);
}
template <typename T>
Bronek marked this conversation as resolved.
Show resolved Hide resolved
requires(std::is_same_v<T, STAmount> || std::is_same_v<T, STIssue>)
SOElement(
TypedField<STAmount> const& fieldName,
TypedField<T> const& fieldName,
SOEStyle style,
SOETxMPTAmount supportMpt = soeMPTNotSupported)
SOETxMPTIssue supportMpt = soeMPTNotSupported)
: sField_(fieldName), style_(style), supportMpt_(supportMpt)
{
init(fieldName);
Expand All @@ -93,7 +95,7 @@ class SOElement
return style_;
}

SOETxMPTAmount
SOETxMPTIssue
supportMPT() const
{
return supportMpt_;
Expand Down
5 changes: 0 additions & 5 deletions include/xrpl/protocol/STAmount.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,6 @@

namespace ripple {

template <typename A>
concept AssetType =
std::is_same_v<A, Asset> || std::is_convertible_v<A, Issue> ||
std::is_convertible_v<A, MPTIssue> || std::is_convertible_v<A, MPTID>;

// Internal form:
// 1: If amount is zero, then value is zero and offset is -100
// 2: Otherwise:
Expand Down
74 changes: 51 additions & 23 deletions include/xrpl/protocol/STIssue.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
#define RIPPLE_PROTOCOL_STISSUE_H_INCLUDED

#include <xrpl/basics/CountedObject.h>
#include <xrpl/protocol/Issue.h>
#include <xrpl/protocol/Asset.h>
#include <xrpl/protocol/SField.h>
#include <xrpl/protocol/STBase.h>
#include <xrpl/protocol/Serializer.h>
Expand All @@ -31,31 +31,40 @@
class STIssue final : public STBase, CountedObject<STIssue>
{
private:
Issue issue_{xrpIssue()};
Asset asset_{xrpIssue()};
Bronek marked this conversation as resolved.
Show resolved Hide resolved

public:
using value_type = Issue;
using value_type = Asset;

STIssue() = default;

explicit STIssue(SerialIter& sit, SField const& name);

explicit STIssue(SField const& name, Issue const& issue);
template <AssetType A>
explicit STIssue(SField const& name, A const& issue);

explicit STIssue(SField const& name);

Issue const&
issue() const;
template <ValidIssueType TIss>
TIss const&
get() const;

Issue const&
template <ValidIssueType TIss>
bool
holds() const;

value_type const&
value() const noexcept;

void
setIssue(Issue const& issue);
setIssue(Asset const& issue);

SerializedTypeID
getSType() const override;

std::string
getText() const override;

Json::Value getJson(JsonOptions) const override;

void
Expand All @@ -76,35 +85,54 @@
friend class detail::STVar;
};

template <AssetType A>
STIssue::STIssue(SField const& name, A const& asset)
: STBase{name}, asset_{asset}
{
if (holds<Issue>() && !isConsistent(asset_.get<Issue>()))
Throw<std::runtime_error>(

Check warning on line 93 in include/xrpl/protocol/STIssue.h

View check run for this annotation

Codecov / codecov/patch

include/xrpl/protocol/STIssue.h#L93 

Added line #L93 was not covered by tests
Bronek marked this conversation as resolved.
Show resolved Hide resolved
"Invalid asset: currency and account native mismatch");
}

STIssue
issueFromJson(SField const& name, Json::Value const& v);

inline Issue const&
STIssue::issue() const
template <ValidIssueType TIss>
bool
STIssue::holds() const
{
return std::holds_alternative<TIss>(asset_.value());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be return asset_.holds<TIss>();

}

template <ValidIssueType TIss>
TIss const&
STIssue::get() const
{
return issue_;
if (!holds<TIss>(asset_))
Throw<std::runtime_error>("Asset doesn't hold the requested issue");
return std::get<TIss>(asset_);
}

inline Issue const&
inline STIssue::value_type const&
STIssue::value() const noexcept
{
return issue_;
return asset_;
}

inline void
STIssue::setIssue(Issue const& issue)
STIssue::setIssue(Asset const& asset)
{
if (isXRP(issue_.currency) != isXRP(issue_.account))
if (holds<Issue>() && !isConsistent(asset_.get<Issue>()))
Throw<std::runtime_error>(
"invalid issue: currency and account native mismatch");
"Invalid asset: currency and account native mismatch");

issue_ = issue;
asset_ = asset;
}

inline bool
operator==(STIssue const& lhs, STIssue const& rhs)
{
return lhs.issue() == rhs.issue();
return lhs.value() == rhs.value();
}

inline bool
Expand All @@ -116,19 +144,19 @@
inline bool
operator<(STIssue const& lhs, STIssue const& rhs)
Copy link
Collaborator

@Bronek Bronek Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

could we replace this with friend std::weak_ordering operator<=> as well ? Also, we should not need operator!= anymore

{
return lhs.issue() < rhs.issue();
return lhs.value() < rhs.value();
}

inline bool
operator==(STIssue const& lhs, Issue const& rhs)
operator==(STIssue const& lhs, Asset const& rhs)

Check warning on line 151 in include/xrpl/protocol/STIssue.h

View check run for this annotation

Codecov / codecov/patch

include/xrpl/protocol/STIssue.h#L151 

Added line #L151 was not covered by tests
{
return lhs.issue() == rhs;
return lhs.value() == rhs;

Check warning on line 153 in include/xrpl/protocol/STIssue.h

View check run for this annotation

Codecov / codecov/patch

include/xrpl/protocol/STIssue.h#L153 

Added line #L153 was not covered by tests
}

inline bool
operator<(STIssue const& lhs, Issue const& rhs)
operator<(STIssue const& lhs, Asset const& rhs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

also replace with friend std::weak_ordering operator<=>

{
return lhs.issue() < rhs;
return lhs.value() < rhs;
}

} // namespace ripple
Expand Down
4 changes: 2 additions & 2 deletions include/xrpl/protocol/STXChainBridge.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ STXChainBridge::lockingChainDoor() const
inline Issue const&
STXChainBridge::lockingChainIssue() const
{
return lockingChainIssue_.value();
return lockingChainIssue_.value().get<Issue>();
};

inline AccountID const&
Expand All @@ -182,7 +182,7 @@ STXChainBridge::issuingChainDoor() const
inline Issue const&
STXChainBridge::issuingChainIssue() const
{
return issuingChainIssue_.value();
return issuingChainIssue_.value().get<Issue>();
};

inline STXChainBridge::value_type const&
Expand Down
6 changes: 4 additions & 2 deletions src/libxrpl/protocol/Indexes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
//==============================================================================

#include <xrpl/beast/utility/instrumentation.h>
#include <xrpl/protocol/Asset.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/LedgerFormats.h>
#include <xrpl/protocol/SField.h>
Expand Down Expand Up @@ -414,9 +415,10 @@ nft_sells(uint256 const& id) noexcept
}

Keylet
amm(Issue const& issue1, Issue const& issue2) noexcept
amm(Asset const& issue1, Asset const& issue2) noexcept
{
auto const& [minI, maxI] = std::minmax(issue1, issue2);
auto const& [minI, maxI] =
std::minmax(issue1.get<Issue>(), issue2.get<Issue>());
Bronek marked this conversation as resolved.
Show resolved Hide resolved
return amm(indexHash(
LedgerNameSpace::AMM,
minI.account,
Expand Down
Loading
Loading