Fix a mix up with AccountSet flags

[rikublock]

High Level Overview of Change

• rename AccountSetFlag -> AccountSetAsfFlag; defines asf flags (this could be considered a breaking change)
• add enum AccountSetTfFlag AccountSetFlag (name adjusted, see discussion below); defines transaction tf flags
• update AccountSetFlagInterface to only use transaction tf flags
• update/fix tests accordingly

Context of Change

AccountSet has two types of flags.

• "AccountSet Flags" (prefixed asf), used in SetFlag/ClearFlag field, limited to one per transaction
• "AccountSet Transaction Flags" (prefixed tf), used in common Flags field, multiple possible (bit-flags)

(AccountSetAsfFlag are not bit-field flags and therefore do not require a FlagInterface)

Reference:

• https://xrpl.org/accountset.html#accountset-flags
• https://xrpl.org/transaction-common-fields.html
• https://github.com/XRPLF/rippled/blob/develop/src/ripple/protocol/TxFlags.h

Similar to the implementation found in the xrpl.js framework, this PR creates an enum for each type of flag.

Type of Change

• Bug fix (possibly breaking change which fixes an issue)

Test Plan

• only ran poetry run poe test_unit
• needs better/more testing (and extensive review)

[mvadari]

Why is this PR necessary?

[ckniffen]

Nice catch!

I am in favor of the tf flags and TX_FLAGS which currently has completely incorrect flags listed. That said I don't think AccountSetFlags needs to be renamed.

[rikublock]

Why is this PR necessary?

I think the current implementation is confusing and faulty.

Let me showcase this by example (code taken from unit tests)
https://github.com/XRPLF/xrpl-py/blob/v1.7.0/tests/unit/models/transactions/test_better_transaction_flags.py#L20

        actual = models.AccountSet(
            account=ACCOUNT,
            flags=models.AccountSetFlagInterface(
                asf_account_tx_id=True,
                asf_authorized_nftoken_minter=True,
                asf_default_ripple=True,
                asf_deposit_auth=True,
                asf_disable_master=True,
                asf_disallow_xrp=True,
                asf_global_freeze=True,
                asf_no_freeze=True,
                asf_require_auth=True,
                asf_require_dest=True,
            ),
        )

As outlined before there are two kinds of AccountSet flags, the asf and tf flags. The somewhat special asf flags can take integer values from 1 to 10 (these are just numbers and do not represent bits). At most one asf flag can be set per transaction.

The above code has several issues:

• a FlagInterface essential represent bits; doing bit operations on asf flags makes no sense
• the flags parameter accepts tf transaction flags, assigning the asf gibberish produced by the AccountSetFlagInterface makes even less sense

[rikublock]

Nice catch!

I am in favor of the tf flags and TX_FLAGS which currently has completely incorrect flags listed. That said I don't think AccountSetFlags needs to be renamed.

I think the FlagInterface changes are also very much necessary.

In regards to naming. Currently the pattern used throughout the library for naming flags seems to be tx name + 'Flag'. For example:

• TrustSetFlag for TrustSet
• OfferCreateFlag for OfferCreate

Following that would result in:

• AccountSetFlag for the tf transaction flags
• AccountSetAsfFlag for the asf flags (that's the outlier/special case, should probably also get the outlier name)

The only drawback/consideration would have to be that we are "switching" the flags that were defined in AccountSetFlag (until now asf).

[rikublock]

Assuming this PR moves forward eventually, I will have to make some changes:

• someone (@mvadari ?) has to make a decision regarding the enum names (see above)
• will need a rebase to resolve some minor conflicts introduced in Add disallow_incoming flags #522
• will need minor linting fixes

[justinr1234]

@rikublock would you be able to make those changes now?

@mvadari do you have an opinion on the enum names?

[ckniffen]

• AccountSetFlag for the tf transaction flags
• AccountSetAsfFlag for the asf flags (that's the outlier/special case, should probably also get the outlier name)

I like these names and agree with the logic.

[rikublock]

@rikublock would you be able to make those changes now?

I'll take a look at it this week.

[JST5000]

As a rough plan of action, I think these changes should be included in xrpl-py 2.0, so I'll take it as an action item today/tomorrow to get the xrpl-py 2.0 branch up to date so we can point it at that branch without a lot of unrelated changes appearing in the changelog :)

[rikublock]

I added the outstanding changes and fixes. I also split the commits to make each individual change easier to understand/review.

As a rough plan of action, I think these changes should be included in xrpl-py 2.0, so I'll take it as an action item today/tomorrow to get the xrpl-py 2.0 branch up to date so we can point it at that branch without a lot of unrelated changes appearing in the changelog :)

Sounds good. I agree, it might be best to include this in a major release as the PR contains somewhat breaking changes.

[JST5000]

LGTM - one last thing though could you add a note in the HISTORY.md file about the changes? (I'd recommend including both the name change & bug fix as separate bullets so it's very clear to future readers that there's something they have to do to update)

[JST5000]

@rikublock Ah, didn't realize you were changing things at the moment - I just was rebasing to try to fix the merge conflicts. It seems I have included all the above commits in the final version. Are there any other changes you were hoping to make before we merge it?

[JST5000]

I think I may have misinterpreted the Git history - if there are changes we need to make we can do a follow up PR :)

[rikublock]

@JST5000 As far as I am concerned this PR included all the necessary changes.