feat: classify DIPs proposal rejections by area and nature#1041
feat: classify DIPs proposal rejections by area and nature#1041MoonBoi9001 wants to merge 1 commit into
Conversation
RembrandtK
left a comment
RembrandtK
left a comment
There was a problem hiding this comment.
I have reviewed at leaf level, a good set of options considered.
I wonder about the model of seeking to predict, how to group handling, and how to extend or provide more detail.
Might want to consider a model that splits and groups more by area than at leaf level of individual aspects. Having a MECE set of areas could help, for example the reason could be related to:
- Agreement Timing
- Agreement Conditions
- Agreement Price
- Agreement Metadata
- Agreement Parties
- Agreement Auth
- Escrow
- Subgraph
Might be able to orthogonally group into a set of what can be wrong, under two broad (extendable) categories:
- Invalid
- Failed to retrieve or not available
- Unable to parse
- Incorrect parameters
- Expired or no longer valid
- Declined
- Not accepted (terms/price not accepted, party not accepted, etc)
- Not supported (do not have capability or do not want to perform this type of work)
- Too large/big (permanent capacity rejection)
- Busy (could be be resubmitted later)
Additionally allowing optional extra text detail could help.
It might be not all combinations are applicable, but many would be. An extendable design would allow options to be added without breaking backwards compatibility, so that older software on either side could continue to work with still correct but less granular information.
| REJECT_REASON_PRICE_TOO_LOW = 1; /// The offered price is below the indexer's minimum. | ||
| REJECT_REASON_OTHER = 2; /// Any other reason (bad signature, etc.). | ||
| REJECT_REASON_OTHER = 2; /// Any other reason not covered by a more specific variant. | ||
| REJECT_REASON_SIGNER_NOT_AUTHORISED = 3; /// The proposal signer is not authorised on the escrow contract. |
There was a problem hiding this comment.
Should authorised on the Recurring Agreement Manager, not the escrow contract?
| REJECT_REASON_UNSUPPORTED_NETWORK = 5; /// The subgraph's network is not supported by this indexer. | ||
| REJECT_REASON_SUBGRAPH_MANIFEST_UNAVAILABLE = 6; /// The subgraph manifest could not be fetched from IPFS. | ||
| REJECT_REASON_UNEXPECTED_SERVICE_PROVIDER = 7; /// The RCA service provider does not match this indexer. | ||
| REJECT_REASON_AGREEMENT_EXPIRED = 8; /// The agreement end time has already passed. |
There was a problem hiding this comment.
Not needed as will as REJECT_REASON_DEADLINE_EXPIRED? Might have been picked up to correlate to endsAt rather than deadline', but do not need a reason for endsAt, should never be more restrictive than deadline'.
It might however be worth adding a reason for invalid agreement proposals (that is proposals with invalid values).
| REJECT_REASON_AGREEMENT_EXPIRED = 8; /// The agreement end time has already passed. | ||
| REJECT_REASON_UNSUPPORTED_METADATA_VERSION = 9; /// The metadata version is not supported. | ||
| REJECT_REASON_INVALID_SIGNATURE = 10; /// The proposal's signature failed to verify. | ||
| REJECT_REASON_SENDER_NOT_TRUSTED = 11; /// The proposal sender is not a trusted agreement manager. |
There was a problem hiding this comment.
How does this differ from REJECT_REASON_SIGNER_NOT_AUTHORISED?
| REJECT_REASON_SENDER_NOT_TRUSTED = 11; /// The proposal sender is not a trusted agreement manager. | ||
| REJECT_REASON_CAPACITY_EXCEEDED = 12; /// The indexer has reached its DIPs agreement capacity. | ||
| REJECT_REASON_MANIFEST_TOO_LARGE = 13; /// The subgraph manifest exceeds the maximum allowed size. | ||
| REJECT_REASON_REPLAY_DETECTED = 14; /// The proposal repeats an already-seen message. |
There was a problem hiding this comment.
Should think if idepotency is better and practical, and/or if resending is valid anyway (if a previous rejection was for capacity for example).
For the first Dipper could fail to get a response, try to resend, and get a failure response. In the meantime the indexer could have accepted?
| @@ -46,12 +46,18 @@ enum ProposalResponse { | |||
| enum RejectReason { | |||
| REJECT_REASON_UNSPECIFIED = 0; /// Default / not set (used for ACCEPT responses). | |||
There was a problem hiding this comment.
Is this actually 'ACCEPTED' then, rather than an unspecified rejection? Or just do not define an enum member for 0? Could also logically overlap with REJECT_REASON_OTHER.
| REJECT_REASON_UNSPECIFIED = 0; /// Default / not set (used for ACCEPT responses). | ||
| REJECT_REASON_PRICE_TOO_LOW = 1; /// The offered price is below the indexer's minimum. | ||
| REJECT_REASON_OTHER = 2; /// Any other reason (bad signature, etc.). | ||
| REJECT_REASON_OTHER = 2; /// Any other reason not covered by a more specific variant. |
There was a problem hiding this comment.
Might be worth allowing an optional text field for extra information (regardless of which reason)?
The indexer answers a DIPs proposal over gRPC, but every rejection collapsed into one generic reason, so the sender couldn't tell what was wrong or whether retrying might help. Describe a rejection by two axes -- which part of the agreement and the kind of problem -- plus optional detail, degrading gracefully. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MoonBoi9001
force-pushed
the
mb9/add-dips-grpc-reject-reasons
branch
from
0b4c34e to
ca26d2b
Compare
MoonBoi9001
changed the title
2 participants
TL;DR
Describes why the indexer rejects a DIPs proposal along two axes — which part of the agreement, and what kind of problem — plus an optional detail message, instead of one flat reason. This is the base of the DIPs validation work; the only change here is the shape of the reject response.
Motivation
When the indexer receives a DIPs agreement proposal over gRPC it answers accept or reject, and on a reject it returns a coded reason. With a single flat list of reasons, the sender (the Dipper service) has to recognise each specific value to know what to do, and a value it doesn't recognise tells it nothing — in particular, nothing about whether the proposal is permanently bad or could succeed if it were sent again later.
This describes a rejection along two axes instead: an area (timing, price, metadata, parties, auth, escrow, subgraph) and a nature, grouped into "invalid" — the request itself is bad — versus "declined" — well-formed, but the indexer won't or can't serve it, including a transient "busy". A reader that doesn't recognise a newer value on one axis can still act on the part it understands, and the nature directly signals whether sending it again could help, which is what the Dipper's back-off needs. This follows Rembrandt's review of the original flat-list version.
Summary