Outgoing/Incoming HookOn

[tequdev]

High Level Overview of Change

Instead of making the Outgoing/Incoming txn check within the Hook code, this makes it possible to check it outside the Hook code by defining it in the HookDefinition.

Context of Change

• Add OutgoingHookOn/IncomingHookOn field

• In the HookDefinition field, one of the following fields is set

  1. HookOn
  2. OutgoingHookOn and IncomingHookOn

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)
• 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)

[RichardAH]

I think this is a good idea but should we perhaps just add one new field OutgoingHookOn. If it's absent then HookOn is used for both (as you've already coded here) but if the new field is present then outgoing is filtered through it. A little bit less confusing than adding two new fields?

[tequdev]

I think this is a good idea but should we perhaps just add one new field OutgoingHookOn. If it's absent then HookOn is used for both (as you've already coded here) but if the new field is present then outgoing is filtered through it. A little bit less confusing than adding two new fields?

When considering a scenario where a HookDefinition created by another account has OutgoingHookOn set,
how can we distinguish between:

1. The case where you want to use the same value for OutgoingHookOn as Definition in your own Hook object
2. The case where you want to use only HookOn for a common value for both incoming and outgoing without setting OutgoingHookOn

[RichardAH]

When considering a scenario where a HookDefinition created by another account has OutgoingHookOn set, how can we distinguish between:

1. The case where you want to use the same value for OutgoingHookOn as Definition in your own Hook object
2. The case where you want to use only HookOn for a common value for both incoming and outgoing without setting OutgoingHookOn

I think if OutgoingHookOn is specified in the definition and you install that hook, but don't specify it, then you'd use the definition's OutgoingHookOn. It's up to you to know what's in the definition. If you want your HookOn field to be the same as OutgoingHookOn then the safest thing to do is actually specify a OutgoingHookOn (identical to HookOn) every time. The backwards compatibility mode is only there to prevent stuff from breaking. People shouldn't use it unless they have to.

[tequdev]

@RichardAH

Do you have any thoughts on using IncomingHookOn instead of OutgoingHookOn?

The Functional Hooks currently under development can only be called from Incoming Txn, so I think it would be better to use the name IncomingHookOn here for this feature.

[RichardAH]

@RichardAH

Do you have any thoughts on using IncomingHookOn instead of OutgoingHookOn?

The Functional Hooks currently under development can only be called from Incoming Txn, so I think it would be better to use the name IncomingHookOn here for this feature.

I think it fine as long as its well documented. So in the absence of IncomingHookOn field the HookOn field is both (incoming and outgoing) but if you add IncomingHookOn then HookOn only becomes the outgoing.

[RichardAH]

Suggest changing names to HookOnIncoming HookOnOutgoing... following general convention of "name" "action" in both hook apis and transactors

[RichardAH]

Negative A or Negative (B and C) is not intuitive. Prefer positive rather than negative logic, or apply De morgans law (Negative A or Negative B or Negative C) for ease of reading and to help future programmers not make a logic error here.

Best practice is make the condition bigger and easier to read like this:

if (enabled(featureHookOnV2))
{
  if (isFieldPresent(sfHookOnOutgoing) && isFieldPresent(sfHookOnIncoming))
  {
    // pass, this is valid in v2
  }
  else
  {
      // error here
  }
}