move tfClawTwoAssets check to preflight

[yinyiqian1]

High Level Overview of Change

Context of Change

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactor (non-breaking change that only restructures code)
• Performance (increase or change in throughput and/or latency)
• Tests (you added tests for code that already exists, or your new feature included in this PR)
• Documentation update
• Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
• Release

API Impact

• Public API: New feature (new methods and/or new fields)
• Public API: Breaking change (in general, breaking changes should only impact the next api_version)
• libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
• Peer protocol change (must be backward compatible or bump the peer protocol version)

[gregtatcam]

LGTM

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.9%. Comparing base (f419c18) to head (b54d85d).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #5201   +/-   ##
=======================================
  Coverage     77.9%   77.9%           
=======================================
  Files          784     784           
  Lines        66680   66681    +1     
  Branches      8138    8140    +2     
=======================================
+ Hits         51942   51948    +6     
+ Misses       14738   14733    -5     

Files with missing lines	Coverage Δ
src/xrpld/app/tx/detail/AMMClawback.cpp	100.0% <100.0%> (ø)

... and 4 files with indirect coverage changes

[shawnxie999]

Although changing to temMALFORMED works, I personally prefer using temINVALID_FLAG since it gives clearer message to the client on which part of the transaction might be wrong. However I don't see this as a blocker so you could decide at your discretion.

[shawnxie999]

Could maybe consider to return temINVALID_FLAG maybe?

[yinyiqian1]

It should be temMALFORMED. The flag is valid but the issuer is not the same.

I used temINVALID_FLAG in my first commit, @gregtatcam adviced to change from temINVALID_FLAG to temMALFORMED.

[gregtatcam]

This is what the requestor of this issue suggested. I'm fine with temINVALID_FLAG. How about temBAD_ISSUER? After all this is what it's pretty much is.

[gregtatcam]

But if we change this to temBAD_ISSUER then I think we should change to the same error this check

if (asset.account != issuer)

[ximinez]

I have to disagree here. We really should get away from using what I've been calling "redundant" error codes. tem means malformed, so temMALFORMED means "malformed malformed". (And before anyone points it out, I know it's used in a ton of places in the existing code, but that doesn't mean it's correct here.) This is especially true when there are other good options that already exist.

• temINVALID_FLAG has already been mentioned. It makes sense because you're not allowed to use that flag when the AMM has assets from two different issuers. One way to look at it is that tfClawTwoAssets is a flag defined for a very specific special case - when the AMM has two different assets from the same issuer and the issuer wants to get both of them, instead of the default behavior of just one. Thus, it is invalid to use the flag when the issuers are different.
  • The two assets are properties of the AMM. They're not going to change. If the AMM was created with assets from two different issuers, then there is nothing you can change about the transaction to work with that AMM and use tfClawTwoAssets.
• temBAD_AMM_TOKENS could be considered, though the error message would need to be updated. The transaction can't be processed with the given flags because the tokens are not compatible with it.
• temBAD_ISSUER is another option. The flag can not be used with the issuer of asset2.
• temINVALID_ACCOUNT_ID is a stretch, but it could be considered invalid to specify that an asset2 account ID that is not the same as asset.

Of these four options, though, I think temINVALID_FLAG is the best, and is better than temMALFORMED because at least it communicates some information about the problem.

[yinyiqian1]

OK. Since everyone agrees on temINVALID_FLAG, I changed the return error to temINVALID_FLAG

[shawnxie999]

@gregtatcam From my perspective, the flag can only be set if the issuer of the two assets are the same. So the flag would be considered as "invalid" if the issuers are different. I believe I did something similar in MPTokenIssuanceSet, where I returned temINVALID_FLAG if both flags are set. I don't believe there is a written rule that says we can only return this error code when the flag mask is invalid.

And we may not want to introduce additional error codes like temBAD_ISSUER since there is an upper limit to the total number of err codes.

Just my 2 cents

[gregtatcam]

Sounds good.

temBAD_ISSUER is an existing error.

[shawnxie999]

Thanks for bearing the suggestions! :)

[yinyiqian1]

@ximinez Pipeline failed with some unit test errors, but it's transient because the error is RR:Env Env::close() failed: no response from server. Running the tests locally is fine. Do you want to rerun it?