Create a Quarantine module

[SpicyLemon]

Description

closes: provenance-io/provenance#1046

This PR creates a quarantine module and ties it into the bank module. Any time funds are transferred to a quarantined account, they are instead routed to a fund holder. The quarantined account must then accept them before they finish their journey.

Accounts must opt in to quarantine, but can opt out later.

Funds can be declined. Declined funds will sit with the account holder, but won't show up in general query results. Declined funds can still be accepted.

Minimal changes were made to the bank module, and the bank module will function normally in the absence of the quarantine module.

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[SpicyLemon]

Since I changed a few things again...

Reran tests and sims, the following commands run without error:

$ go clean -testcache
$ make test
$ make test-sim-nondeterminism
$ make test-sim-import-export
$ make test-sim-after-import
$ make test-sim-multi-seed-short

[egaxhaj]

This currently points to prov/dwedul/1046-bank-quarantine branch. They'll break after merge when the branch is deleted. Do we update this after merge to point to main?

[SpicyLemon]

Yeah. We'll need to update them. They'll be okay as long as the branch isn't deleted (even after getting merged).

Follow-up ticket for that: provenance-io/provenance#1312

[egaxhaj]

This should just be off.

Suggested change

	"unspecified", "u", "off", or "o" - turn off auto-responses for the following <from_address>(es).
	"off", or "o" - turn off auto-responses for the following <from_address>(es).

[SpicyLemon]

Since the enum value is unspecified, that should be an option here too.

[egaxhaj]

I understand why the guides suggest the suffix and it makes sense at the proto message level since that is the default value for unexpected unum values but do we have to expose unspecified to the end user? It's not a good UX IMO. We can stick with the three states accept, decline and off, they're clear and concise.

The zero value enum should have the suffix UNSPECIFIED, because a server or application that gets an unexpected enum value will mark the field as unset in the proto instance. The field accessor will then return the default value, which for enum fields is the first enum value.

[SpicyLemon]

Since the enum field is named unspecified, and these strings correlate with auto-response enum entries, it needs to be an option as a string.

If we take out unspecified and u, leaving only off and o:

• It's not as obvious what off or o translates to.
• If you know you want an AUTO_RESPONSE_UNSPECIFIED, it's clear what you need to do.

By calling a thing the same in all places, it makes it easier to use. Honestly, I was torn on allowing off or o here, but decided to include them due to the nicer semantics.

Why do you think it's not good UX?

Would it be enough to just reorder them like this:

Suggested change

	"unspecified", "u", "off", or "o" - turn off auto-responses for the following <from_address>(es).
	"off", "o", "unspecified", or "u", - turn off auto-responses for the following <from_address>(es).

[egaxhaj]

Why do you think it's not good UX?

Would it be enough to just reorder them like this:

It's not the reordering I'm concerned with, it's the UX. unspecified represents an initial state. Meaning the user hasn't not yet made a selection for this service. What happens when the user opt-in to accept or decline and then wants to disable auto-responses? The service is no longer in an unspecified state, it is in an off state. Also, the description reads turn off auto-responses for the following <from_address>(es)

By calling a thing the same in all places, it makes it easier to use. Honestly, I was torn on allowing off or o here, but decided to include them due to the nicer semantics.

We can more easily define the auto-response service with three states; accept, decline and off - default. It makes for a better UX and nicer semantics (we're all quite familiar to off).

What about creating middle ground between UX and proto guidelines by mapping val AUTO_RESPONSE_OFF = AUTO_RESPONSE_UNSPECIFIED in the code?

If none of the above doesn't sound right that consider using only unspecified and remove off. "unspecified", or "u", - no auto-response action selected for the following <from_address>(es). or something like that.

[SpicyLemon]

In CLI, the easier it is to guess the words and phrases of a command, the easier it is to use the command. The unspecified option should be available since that's what the enum entry is named, so will sometimes be what people expect to use. The most common driving thought, though, will be, "I need to turn off an auto-response," so it makes sense to have 'off' be an option too.

What about creating middle ground between UX and proto guidelines by mapping val AUTO_RESPONSE_OFF = AUTO_RESPONSE_UNSPECIFIED in the code?

The protos drive everything. They're the root source of API documentation, and generating code from them is the first step in using our blockchain. The CLI stuff, specially tx and query pieces, are just wrappers on the gRPC calls defined by the protos. Defining that alias would only affect the go code, though, leading to one name used in our go code, and another used everywhere else.

What happens when the user opt-in to accept or decline and then wants to disable auto-responses?

Opting in or out is separate from auto-responses. A user opts themselves into quarantine, then they can set up auto-responses for funds coming from specific other addresses if they want to. When they opt in, an opt-in record is created for the receiver. When they define an auto-response action to take from a sender, an auto-response record is created indicating the receiver, sender, and whether it's auto-accept or auto-decline. To disable the auto-response to a receiver from a sender, that auto-response record is deleted.

unspecified represents an initial state.

This isn't quite accurate. Yes, an initial state is unspecified, but unspecified does not mean the thing is in an initial state; things can go back to unspecified.

The service is no longer in an unspecified state.

When setting an auto-response to unspecified or off, it deletes the appropriate auto-response record, so it actually does go back to an unspecified state.

---

I feel like the primary driving thought that will lead to use of this option in the command is, "I need to turn off my auto-responses from an address." From there, there's two paths someone can take:

1. They start with the CLI, looking at provenanced tx quarantine update-auto-responses --help. They will see that off is an option, and use that in the command they end up running.
2. They look up the protos to see how to update auto-responses. That would lead them to the UpdateAutoResponses tx endpoint which involves a list of AutoResponseUpdates. That message has a comment on the response field indicating that AUTO_RESPONSE_UNSPECIFIED is what they want. From there, they might use the CLI, but they might use something else. Either way, they are now looking to use unspecified now.

---

I updated the comment in AutoResponseUpdate on the response field:

-  // response is the automatic action to take on funds sent from from_address, or in the
-  // case of AUTO_RESPONSE_UNSPECIFIED, that no automatic action should be taken.
+  // response is the automatic action to take on funds sent from from_address.
+  // Provide AUTO_RESPONSE_UNSPECIFIED to turn off an auto-response.
   AutoResponse response = 2;

I also updated the comment on the AUTO_RESPONSE_UNSPECIFIED enum entry:

-  // AUTO_RESPONSE_UNSPECIFIED defines that no automatic action should be taken.
+  // AUTO_RESPONSE_UNSPECIFIED defines that an automatic response has not been specified.
+  // This means that no automatic action should be taken, i.e. this auto-response is off,
+  // and default quarantine behavior is used.
   AUTO_RESPONSE_UNSPECIFIED = 0;

[egaxhaj]

👏 The comments are much better now.

1. They start with the CLI, looking at provenanced tx quarantine update-auto-responses --help. They will see that off is an option, and use that in the command they end up running.

This is why I think that unspecified doesn't really need to be an option on the CLI. The choice almost always will be off. We can add comments to the --help and specify that off maps to AUTO_RESPONSE_UNSPECIFIED in the protos.

[SpicyLemon]

There's no reason to not have both.

Allowing for off:

1. Matches what more often comes to mind.
2. Is easier to type than "unspecified".
3. Reads better (to a human) in a command context.

Allowing for unspecified:

1. Auto-documents that off actually uses unspecified. This is crucial for someone that starts by looking at the CLI in order to make plans for their own program/use.
2. Allows a user to use the name of the thing that is actually being used. Even if this is rare case, this is crucial.

Even if we expect it to almost always be "off" here, we still need "unspecified" as an option because that's what it actually is. To not have "unspecified" does a disservice to anyone that uses the CLI as well as any other method of interacting with provenance. Without it, there's a disparity between what its called depending on context.

The only place where it'll be aliased as "off" is the CLI, and that's okay. What's not okay is ONLY calling it "off" in the CLI, when it's called "unspecified" everywhere else.

Personally, I get annoyed when I know that I need a thing, but the interface I'm using calls it something different. It forces me to stop and translate what I know into something else which the interface is just going to convert back to what I already knew I wanted. It wastes my time and I don't appreciate it.

[egaxhaj]

LGTM