server_definitions should include `DeliverMax`

[mDuo13]

Issue Description

The server_definitions API response includes a list of field names to field IDs, but the outputted definitions omits DeliverMax because it's a special case. (This is true regardless of whether you specify API v2+ or not.)

If you use the generated definitions.json in client-side signing code, you'll get an error when clients provide a DeliverMax field unless your client-side code also includes a special case for this field.

Including an entry for DeliverMax in the FIELDS list would make client JSON-to-binary serialization work "naively" correctly when given transactions in either format (though it wouldn't necessarily catch cases where both field names are provided).

Binary to JSON conversion will still need a special case for DeliverMax since the "proper" field name depends on which transaction type it's in, but that's already the case.

[ckeshava]

I was of the understanding that server_definitions portrays the peer-to-peer communication standard between any two nodes in the XRPL blockchain. That being the case, DeliverMax is not a recognized serialized-field in the communication protocol. Deserializing a tx_blob will still render the field-name as Amount, not DeliverMax.

How can we go about this solution? DeliverMax does not have a field_code (or) a type_code. isSerialized, isSigning, isVariable-Length-Encoded -- these questions are not appropriate for the DeliverMax field.

However, I do agree with the larger pain-point that you mention in this issue description. I'm supportive of making DeliverMax a first-class citizen amongst the other serialized fields, i.e. sfDeliverMax similar to sfAmount. That would encompass the suggestions you make (a place for sfDeliverMax inside server_definitions). However, it would also be a breaking change in the serialization protocol (with respect to the Payment transactions)

[zgrguric]

DeliverMax should be added to server_definitions, same as DeliverMin is there.

[dangell7]

DeliverMax should be added to server_definitions, same as DeliverMin is there.

DeliverMax does not exist in the binary codec therefore it should not go in the server definitions. It doesn't have a type or field code. DeliverMin is a valid SField

[Bronek]

DeliverMax should be added to server_definitions, same as DeliverMin is there.

What @dangell7 has said. I will also add that DeliverMax is not a field; it is an alias to Amount. The client side RPC libraries ought to support it, but we do not have the means to express it in server_definitions.

[zgrguric]

Okay, I would suggest idea of adding new category “FIELD_ALIASES” and describe those fields in that seperate category.

[mvadari]

server_definitions is specific to only what's necessary for serialization/deserialization/encoding/decoding. IMO having a FIELD_ALIASES category is out of scope - it's only an RPC-level feature.

To provide a better example: a second client implementation doesn't need to know anything about DeliverMax and would be able to participate in consensus just fine.

[mDuo13]

Technically the server_definitions method / definitions.json file are just shortcuts/documentation for making client implementations easier. Not having the preferred name for this field in the list adds a complication that client libraries have to deal with. Just having an entry for DeliverMax in the FIELDS array would simplify some things. There needs to be a special case somewhere, so it's more about whether it's only in the server or every client library too.

Client libraries don't participate in consensus at all, but it's true that an alternate server implementation could participate in consensus without knowing about DeliverMax... It could do so without knowing about server_definitions at all. In fact, the server worked for years without this method and the client libraries just had to implement and maintain their own field mappings. It worked, but we created server_definitions to improve on that situation, as an automatic way to define and publish a standard for converting JSON to binary.

It just seems like a step backwards to require additional information that's "off the record" instead of just including it in the standard API.

[mvadari]

Technically the server_definitions method / definitions.json file are just shortcuts/documentation for making client implementations easier.

Yes, for serialization/deserialization/etc. DeliverMax is not an SField, it doesn't have serialization info to be included in server_definitions. It's not accepted in a Payment transaction (or any other transaction, for that matter).

We don't include other synthetic RPC fields like nftoken_id in server_definitions either.

~~The client libraries don't even support submitting transactions with DeliverMax: https://github.com/XRPLF/xrpl.js/blob/main/packages/xrpl/src/models/transactions/payment.ts#L212-L216~~ This was a bug, sorry