Add allowClawback flag for account_info
#4590
Conversation
kennyzlei
requested review from
ckniffen,
nbougalis,
seelabs,
ckeshava and
ledhed2222
and removed request for
ckniffen
| @@ -607,6 +632,8 @@ class AccountInfo_test : public beast::unit_test::suite | |||
| ripple::test::jtx::supported_amendments()}; | |||
| testAccountFlags(allFeatures); | |||
| testAccountFlags(allFeatures - featureDisallowIncoming); | |||
| testAccountFlags( | |||
| allFeatures - featureDisallowIncoming - featureClawback); | |||
There was a problem hiding this comment.
why are we removing the featureClawback from set of all features? Shouldn't that be enabled for us to run the unit tests?
There was a problem hiding this comment.
Amendments are included by default I think. So it has already been tested on lines 633 and 634
| @@ -508,13 +508,16 @@ class AccountInfo_test : public beast::unit_test::suite | |||
|
|
|||
| Env env(*this, features); | |||
| Account const alice{"alice"}; | |||
| env.fund(XRP(1000), alice); | |||
| Account const bob{"bob"}; | |||
| env.fund(XRP(1000), alice, bob); | |||
There was a problem hiding this comment.
is it required to fund bob's account in order to test the account flags?
Also, shouldn't the clawback feature be tested with a custom token (different from XRP) ?
There was a problem hiding this comment.
Custom token is only needed for trustlines, we don’t do that here. This line is to just fund bob so that it becomes a valid account
There was a problem hiding this comment.
okay. But the clawback feature is not applicable for XRP token asset right?
Isn't it better to perform this check on an account with custom tokens, instead of XRP tokens?
It appears that we are only checking if certain flags are set/unset, so I guess it doesn't matter.
|
it looks good to me. it's strange that only the macOS workflow is failing, but I couldn't access the logs. |
Attempt # 2 passed. I guess the earlier run failed due to either a glitch in GitHub, or because the macOS runner wasn't available at the time. |
|
@intelliot it failed because of the github downtime last week :-) It's ready to merge. |
* Update the `account_info` API so that the `allowClawback` flag is
included in the response.
* The proposed `Clawback` amendement added an `allowClawback` flag in
the `AccountRoot` object.
* In the API response, under `account_flags`, there is now an
`allowClawback` field with a boolean (`true` or `false`) value.
* For reference, the XLS-39 Clawback implementation can be found in
XRPLF#4553
Fix XRPLF#4588
* Update the `account_info` API so that the `allowClawback` flag is
included in the response.
* The proposed `Clawback` amendement added an `allowClawback` flag in
the `AccountRoot` object.
* In the API response, under `account_flags`, there is now an
`allowClawback` field with a boolean (`true` or `false`) value.
* For reference, the XLS-39 Clawback implementation can be found in
XRPLF#4553
Fix XRPLF#4588
* Update the `account_info` API so that the `allowClawback` flag is
included in the response.
* The proposed `Clawback` amendement added an `allowClawback` flag in
the `AccountRoot` object.
* In the API response, under `account_flags`, there is now an
`allowClawback` field with a boolean (`true` or `false`) value.
* For reference, the XLS-39 Clawback implementation can be found in
XRPLF#4553
Fix XRPLF#4588
High Level Overview of Change
Clawback introduced an
allowClawbackflag forAccountRootobject. This PR updates theaccount_infoAPI so that this flag is reflected in the API response.Solves issue #4588
Context of Change
(Clawback implementation can be found here)
Example