Addressing some comments from a peer review.

ratelimits/bucket.go

@@ -17,8 +17,8 @@ type bucketId struct {
 	// looking up default limits.
 	limitName Name
 
-	// bucketKey is the limit Name enum (e.g. "1") concatenated with the
-	// subscriber identifier specific to the associate limit Name type.
+	// bucketKey is the limit Name enum (e.g. "1") concatenated with the a
+	// subscriber identifier specific to the associated limit Name type.
 	bucketKey string
 }
 
@@ -96,87 +96,112 @@ func newFQDNSetBucketId(name Name, orderNames []string) (bucketId, error) {
 	}, nil
 }
 
-// Transaction is a cost to be spent or refunded from a specific bucket
-// identified by the bucketId.
+// Transaction is used to represent a single transaction against a rate limit.
+// It is used by the Limiter to determine if a transaction should be denied or
+// allowed.
 type Transaction struct {
 	bucketId
+
+	// cost is the amount of capacity to be spent or refunded from the bucket.
+	// It must be >= 0.
 	cost int64
 
-	// optimistic indicates to the limiter that the cost should be spent if
-	// possible, but should not be denied if the bucket lacks the capacity to
-	// satisfy the cost.
-	optimistic bool
+	// check indicates to the limiter that the cost should be checked against
+	// the bucket's capacity. If check is false, the transaction is considered
+	// spend-only. Meaning, the transaction will not be denied if the bucket
+	// lacks the capacity to satisfy the cost.
+	check bool
+
+	// spend indicates to the limiter that the cost should be spent from the
+	// bucket's capacity, if possible. If spend is false, the transaction is
+	// considered check-only.
+	spend bool
+}
+
+// checkOnly returns true if the transaction is check-only. Check-only
+// transactions will never have their cost deducted from or refunded to the
+// bucket's capacity.
+func (t Transaction) checkOnly() bool {
+	return t.check && !t.spend
+}
 
-	// checkOnly indicates to the limiter that the cost should be checked but
-	// not spent or refunded.
-	checkOnly bool
+// spendOnly returns true if the transaction is spend-only. Spend-only
+// transactions will NOT be denied if the bucket lacks the capacity to satisfy
+// the cost.
+func (t Transaction) spendOnly() bool {
+	return t.spend && !t.check
 }
 
 // newTransaction creates a new Transaction for the provided BucketId and cost.
 func newTransaction(b bucketId, cost int64) Transaction {
 	return Transaction{
 		bucketId: b,
 		cost:     cost,
+		check:    true,
+		spend:    true,
 	}
 }
 
 // newCheckOnlyTransaction creates a new check-only Transaction for the provided
-// BucketId and cost. Check-only transactions will not have their cost deducted
-// from the bucket's capacity.
+// BucketId and cost. Check-only transactions will never have their cost
+// deducted from or refunded to the bucket's capacity.
 func newCheckOnlyTransaction(b bucketId, cost int64) Transaction {
 	return Transaction{
-		bucketId:  b,
-		cost:      cost,
-		checkOnly: true,
+		bucketId: b,
+		cost:     cost,
+		check:    true,
 	}
 }
 
-// newOptimisticTransaction creates a new optimistic Transaction for the
-// provided BucketId and cost. Optimistic transactions will not be denied if the
-// bucket lacks the capacity to satisfy the cost.
-func newOptimisticTransaction(b bucketId, cost int64) Transaction {
+// newSpendOnlyTransaction creates a new spend-only Transaction for the provided
+// BucketId and cost. Spend-only transactions will NOT be denied if the bucket
+// lacks the capacity to satisfy the cost.
+func newSpendOnlyTransaction(b bucketId, cost int64) Transaction {
 	return Transaction{
-		bucketId:   b,
-		cost:       cost,
-		optimistic: true,
+		bucketId: b,
+		cost:     cost,
+		spend:    true,
 	}
 }
 
-// NewRegistrationsPerIPAddressTransaction returns a Transaction for the
+// RegistrationsPerIPAddressTransaction returns a Transaction for the
 // NewRegistrationsPerIPAddress limit for the provided IP address.
-func NewRegistrationsPerIPAddressTransaction(ip net.IP, cost int64) (Transaction, error) {
+func RegistrationsPerIPAddressTransaction(ip net.IP, cost int64) (Transaction, error) {
 	bucketId, err := newIPAddressBucketId(NewRegistrationsPerIPAddress, ip)
 	if err != nil {
 		return Transaction{}, err
 	}
 	return newTransaction(bucketId, cost), nil
 }
 
-// NewRegistrationsPerIPv6RangeTransaction returns a Transaction for the
+// RegistrationsPerIPv6RangeTransaction returns a Transaction for the
 // NewRegistrationsPerIPv6Range limit for the /48 IPv6 range which contains the
 // provided IPv6 address.
-func NewRegistrationsPerIPv6RangeTransaction(ip net.IP, cost int64) (Transaction, error) {
+func RegistrationsPerIPv6RangeTransaction(ip net.IP, cost int64) (Transaction, error) {
 	bucketId, err := newIPv6RangeCIDRBucketId(NewRegistrationsPerIPv6Range, ip)
 	if err != nil {
 		return Transaction{}, err
 	}
 	return newTransaction(bucketId, cost), nil
 }
 
-// NewOrdersPerAccountTransaction returns a Transaction for the provided ACME
-// NewOrdersPerAccount limit for the provided ACME registration Id.
-func NewOrdersPerAccountTransaction(regId, cost int64) (Transaction, error) {
+// OrdersPerAccountTransaction returns a Transaction for the NewOrdersPerAccount
+// limit for the provided ACME registration Id.
+func OrdersPerAccountTransaction(regId, cost int64) (Transaction, error) {
 	bucketId, err := newRegIdBucketId(NewOrdersPerAccount, regId)
 	if err != nil {
 		return Transaction{}, err
 	}
 	return newTransaction(bucketId, cost), nil
 }
 
-// NewFailedAuthorizationsPerAccountCheckOnlyTransaction returns a 0 cost
+// FailedAuthorizationsPerAccountCheckOnlyTransaction returns a 0 cost
 // check-only Transaction for the provided ACME registration Id for the
-// FailedAuthorizationsPerAccount limit.
-func NewFailedAuthorizationsPerAccountCheckOnlyTransaction(regId int64) (Transaction, error) {
+// FailedAuthorizationsPerAccount limit. This check-only Transaction is used to
+// determine if the limit is exceeded without actually spending any of the
+// bucket's capacity. This is used to determine if a subscriber should be
+// allowed to create a new order and thus new authorizations.
+func FailedAuthorizationsPerAccountCheckOnlyTransaction(regId int64) (Transaction, error) {
 	bucketId, err := newRegIdBucketId(FailedAuthorizationsPerAccount, regId)
 	if err != nil {
 		return Transaction{}, err
@@ -195,17 +220,14 @@ func NewFailedAuthorizationsPerAccountTransaction(regId, cost int64) (Transactio
 }
 
 // NewCertificatesPerDomainTransactions returns a slice of Transactions for the
-// CertificatesPerDomain limit for the provided order domain names.
-//
-// Note: when overrides to the CertificatesPerDomainPerAccount are configured
-// for the subscriber, the cost:
-//   - MUST be consumed from the CertificatesPerDomainPerAccount bucket and
-//   - SHOULD be consumed from each CertificatesPerDomain bucket, if possible.
-//
-// When a CertificatesPerDomainPerAccount override is configured, all of the
-// CertificatesPerDomain transactions returned by this function will be marked
-// as optimistic and the combined cost of all of these transactions will be
-// specified in a CertificatesPerDomainPerAccount transaction as well.
+// for the provided order domain names. An error is returned if any of the order
+// domain names are invalid. When a CertificatesPerDomainPerAccount override is
+// configured, two types of Transactions are returned:
+//   - Regular CertificatesPerDomain Transaction(s), which will NOT be denied if the
+//     bucket lacks the capacity to satisfy the cost, and
+//   - a CertificatesPerDomainPerAccount Transaction, which will be denied if the
+//     bucket lacks the capacity to satisfy the cost of the combined regular
+//     CertificatesPerDomain Transaction(s).
 func NewCertificatesPerDomainTransactions(limiter *Limiter, regId int64, orderDomains []string, cost int64) ([]Transaction, error) {
 	certsPerDomainPerAccountId, err := newRegIdBucketId(CertificatesPerDomainPerAccount, regId)
 	if err != nil {
@@ -229,7 +251,7 @@ func NewCertificatesPerDomainTransactions(limiter *Limiter, regId int64, orderDo
 		if certsPerDomainPerAccountLimit.isOverride {
 			// SHOULD be consumed from each CertificatesPerDomain bucket, if
 			// possible.
-			txns = append(txns, newOptimisticTransaction(certsPerDomainId, cost))
+			txns = append(txns, newSpendOnlyTransaction(certsPerDomainId, cost))
 		} else {
 			txns = append(txns, newTransaction(certsPerDomainId, cost))
 		}