-
Notifications
You must be signed in to change notification settings - Fork 970
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
Clawback Operation #2881
Clawback Operation #2881
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.
Reviewed everything but the tests.
@@ -18,9 +18,11 @@ namespace stellar | |||
{ | |||
|
|||
static const uint32 allAccountFlags = | |||
(AUTH_REQUIRED_FLAG | AUTH_REVOCABLE_FLAG | AUTH_IMMUTABLE_FLAG); | |||
(AUTH_REQUIRED_FLAG | AUTH_REVOCABLE_FLAG | AUTH_IMMUTABLE_FLAG | |
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.
After reviewing the way that trustLineFlagIsValid
was done in the next commit, I'm also questioning whether we should be using a similar approach for the MASK_ACCOUNT_FLAGS
. I think it would probably be slightly better in that the invariant would be able to check for flags that were introduced in later versions when running earlier versions, which is valuable for ensuring correctness in parallel catchup. What do you think?
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 that the reason we could not check this in the bucket entries invariant was that when we apply buckets, we do not know the protocol version (or at least the code was not written in a friendly way) - if it's too involved we should open an issue to address this separately from this PR
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 if you mean that there should be a accountFlagIsValid
equivalent to trustLineFlagIsValid
that we can use both invariants and in setOptions
that makes sense to me
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.
The last commit (bf6bf6e068b7ad24dc3d7418c444cd89b04a5c3e) adds the account flag validity check to LedgerEntryIsValid
, but it works a little different from trustline flag validation.
We can reuse trustLineFlagIsValid
in the invariants and AllowTrustOpFrame::doCheckValid
because mAllowTrust.authorize
is the next expected state of the trustline flags. SetOptionsOpFrame
doesn't work the same way because its parameters change specific bits in the flag. Due to this, we need three different checks -
- At validation - check that parameter bits are valid.
- At apply - check that the revocable/clawback combo is valid.
- Invariants - check that the both conditions above are true.
I kept these changes as a separate commit for now, but will squash it if we think this is worth doing. We will also have a very similar issue with SetTrustlineFlagsOp
, so the pattern used here should be used in the new operation.
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.
The cap will also need to be updated to reflect MASK_ACCOUNT_FLAGS_V16
.
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 wasn't very clear or specific, but as @sisuresh correctly inferred I was talking about the LedgerEntryIsValid
invariant. I agree that any changes to bucket invariants are way out of scope for this PR (but it's an interesting thing to wonder about).
@@ -18,9 +18,11 @@ namespace stellar | |||
{ | |||
|
|||
static const uint32 allAccountFlags = | |||
(AUTH_REQUIRED_FLAG | AUTH_REVOCABLE_FLAG | AUTH_IMMUTABLE_FLAG); | |||
(AUTH_REQUIRED_FLAG | AUTH_REVOCABLE_FLAG | AUTH_IMMUTABLE_FLAG | |
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 that the reason we could not check this in the bucket entries invariant was that when we apply buckets, we do not know the protocol version (or at least the code was not written in a friendly way) - if it's too involved we should open an issue to address this separately from this PR
@@ -224,6 +237,13 @@ SetOptionsOpFrame::doCheckValid(uint32_t ledgerVersion) | |||
innerResult().code(SET_OPTIONS_UNKNOWN_FLAG); | |||
return false; | |||
} | |||
|
|||
if (ledgerVersion < 16 && |
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.
Would it be clearer to just compute allAccountFlags
based on protocol version (as a helper function)
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 removed allAccountFlags
in the last commit (bf6bf6e068b7ad24dc3d7418c444cd89b04a5c3e).
@@ -164,6 +166,17 @@ SetOptionsOpFrame::doApply(AbstractLedgerTxn& ltx) | |||
account.flags = account.flags | *mSetOptions.setFlags; | |||
} | |||
|
|||
// check clawback state after account.flags has changed |
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.
comment should just be explicit on what the invariant is:
ensure that revocable is set if clawback is set
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.
Done
if (mSetOptions.setFlags || mSetOptions.clearFlags) | ||
{ | ||
if ((account.flags & AUTH_CLAWBACK_ENABLED_FLAG) && | ||
(account.flags & AUTH_REVOCABLE_FLAG) == 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.
extra parenthesis for clarity
if ((account.flags & AUTH_CLAWBACK_ENABLED_FLAG) &&
((account.flags & AUTH_REVOCABLE_FLAG) == 0))
or maybe just
if ((account.flags & AUTH_CLAWBACK_ENABLED_FLAG) &&
~(account.flags & AUTH_REVOCABLE_FLAG))
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.
Done
@@ -18,9 +18,11 @@ namespace stellar | |||
{ | |||
|
|||
static const uint32 allAccountFlags = | |||
(AUTH_REQUIRED_FLAG | AUTH_REVOCABLE_FLAG | AUTH_IMMUTABLE_FLAG); | |||
(AUTH_REQUIRED_FLAG | AUTH_REVOCABLE_FLAG | AUTH_IMMUTABLE_FLAG | |
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 if you mean that there should be a accountFlagIsValid
equivalent to trustLineFlagIsValid
that we can use both invariants and in setOptions
that makes sense to me
@@ -847,16 +861,24 @@ setAuthorized(LedgerTxnHeader const& header, LedgerTxnEntry& entry, | |||
bool | |||
trustLineFlagIsValid(uint32_t flag, uint32_t ledgerVersion) | |||
{ | |||
uint32_t invalidAuthCombo = |
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 would prefer to have this in the else (for protocol 13 and later) as this doesn't apply for older versions of the protocol
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.
Done
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.
This is almost ready from my perspective. A few comments from me, then if @MonsieurNicolas is happy with it we can merge.
@@ -18,9 +18,11 @@ namespace stellar | |||
{ | |||
|
|||
static const uint32 allAccountFlags = | |||
(AUTH_REQUIRED_FLAG | AUTH_REVOCABLE_FLAG | AUTH_IMMUTABLE_FLAG); | |||
(AUTH_REQUIRED_FLAG | AUTH_REVOCABLE_FLAG | AUTH_IMMUTABLE_FLAG | |
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 wasn't very clear or specific, but as @sisuresh correctly inferred I was talking about the LedgerEntryIsValid
invariant. I agree that any changes to bucket invariants are way out of scope for this PR (but it's an interesting thing to wonder about).
src/test/TxTests.cpp
Outdated
@@ -1190,6 +1190,27 @@ revokeSponsorship(AccountID const& accID, SignerKey const& key) | |||
return op; | |||
} | |||
|
|||
Operation | |||
clawback(AccountID const& from, Asset const& asset, int64_t amount) |
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.
Nitpick: this interface is a little strange. It entirely ignores the issuer
of asset
, and allows you to specify native (which you didn't handle). You should probably just take AssetCode const&
.
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.
Yeah I can see how the intent isn't as clear as it can be here. I ended up using AssetCode
here, and adding a REQUIRE(asset.type() != ASSET_TYPE_NATIVE);
check above in TestAccount
.
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.
What is the motivation for leaving it as Asset const&
in TestAccount
?
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.
It's simpler to use the existing Asset
instead of creating an AssetCode
off of it. If you feel strongly about this I can change the signature and use a assetToAssetCode
helper method in the tests.
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.
It's test so I don't feel particularly strongly either way. I just worry about things that could be confusing, but I don't think this is so bad.
r+ 5f07723 |
Description
AUTH_CLAWBACK_ENABLED_FLAG
account flag.SET_OPTIONS_AUTH_REVOCABLE_REQUIRED
result code. This is returned ifAUTH_CLAWBACK_ENABLED_FLAG
is set withoutAUTH_REVOCABLE_FLAG
.CLAWBACK_ENABLED_FLAG
trustline flag.AllowTrust
to only modifyAUTHORIZED_FLAG
andAUTHORIZED_TO_MAINTAIN_LIABILITIES_FLAG
.ClawbackOp
.Checklist
clang-format
v5.0.0 (viamake format
or the Visual Studio extension)