Skip to content

Conversation

@cjcobb23
Copy link
Contributor

@cjcobb23 cjcobb23 commented Aug 16, 2022

Previously, we were only checking the ledger sequence to determine if we should insert delivered amount. This works on mainnet, but could fail on other networks that are newer


This change is Reviewable

@cjcobb23 cjcobb23 requested a review from natenichols August 16, 2022 18:35
std::shared_ptr<ripple::TxMeta const> const& meta,
uint32_t date)
{
if (canHaveDeliveredAmount(txn, meta))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to change canHaveDeliveredAmount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think so, that function is just looking at the transaction to see if its the type of transaction that could have a delivered amount.

if (canHaveDeliveredAmount(txn, meta))
{
if (auto amt = getDeliveredAmount(txn, meta, meta->getLgrSeq()))
if (auto amt = getDeliveredAmount(txn, meta, meta->getLgrSeq(), date))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do a cpp17 style if statement 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.

Like : if (auto amt = getDeliveredAmount(txn, meta, meta->getLgrSeq(), date); amt)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. I like that better, but if you hate it feel free to leave it

if (ledgerSequence >= 4594095)
if (ledgerSequence >= 4594095 || date > 446000000)
{
return txn->getFieldAmount(ripple::sfAmount);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it guaranteed that in this case sfAmount will be delivered_amount?

If a transaction is a partial payment, will it always have delivered_amount in the metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the comment (which I copied from rippled), delivered amount will be in the metadata for partial payments after this ledger. The metadata is checked prior to this, at the beginning of the function.

@cjcobb23 cjcobb23 merged commit ac45cce into XRPLF:develop Sep 7, 2022
manojsdoshi pushed a commit to manojsdoshi/clio that referenced this pull request Nov 2, 2022
manojsdoshi pushed a commit to manojsdoshi/clio that referenced this pull request Nov 2, 2022
manojsdoshi pushed a commit to manojsdoshi/clio that referenced this pull request Nov 2, 2022
@manojsdoshi manojsdoshi mentioned this pull request Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants