-
Notifications
You must be signed in to change notification settings - Fork 973
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
CAP-0023 implementation #2591
CAP-0023 implementation #2591
Changes from all commits
770cd83
bba5707
288ceb4
a4a1ea0
74b7f96
901e6bb
ea68303
6c71d56
ba61550
caf454e
36852dd
908cd23
2bb7008
fd0588a
2c44ab3
5161d4d
7f07aa1
7d93e75
0395ebe
34aaa7e
29c1cab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,24 @@ calculateDeltaBalance(std::shared_ptr<LedgerEntry const> const& current, | |
return (current ? current->data.account().balance : 0) - | ||
(previous ? previous->data.account().balance : 0); | ||
} | ||
if (let == CLAIMABLE_BALANCE) | ||
{ | ||
int64_t reserveDelta = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is written as if reserve and amount are mutable, maybe OK in this context (albeit a bit confusing as the code doesn't work at all if asset gets modified) but see my comment in the other invariant on validity There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be simplified when we rebase CAP-33 on top, because the reserve is not stored in that case. But I the code should work correctly in the case that |
||
(current ? current->data.claimableBalance().reserve : 0) - | ||
(previous ? previous->data.claimableBalance().reserve : 0); | ||
|
||
auto const& asset = current ? current->data.claimableBalance().asset | ||
: previous->data.claimableBalance().asset; | ||
|
||
if (asset.type() != ASSET_TYPE_NATIVE) | ||
{ | ||
return reserveDelta; | ||
} | ||
|
||
return ((current ? current->data.claimableBalance().amount : 0) - | ||
(previous ? previous->data.claimableBalance().amount : 0)) + | ||
reserveDelta; | ||
} | ||
return 0; | ||
} | ||
|
||
|
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.
you forgot to update the schema version and to deal with the schema upgrade
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.
you need to wait for #2593 to be merged so that you can rebase on top of it: it's already changing the database schema version
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.
you still need to do this
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.
@MonsieurNicolas I just rebased with master to pull in #2593. Is there anything else I need to do to deal with the schema upgrade for cap-0023?
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 you need to call
mApp.getLedgerTxnRoot().dropClaimableBalances();
fromDatabase::applySchemaUpgrade
when upgrading to schema version13
otherwise the table won't be createdThere 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.
Done