aptos-core icon indicating copy to clipboard operation
aptos-core copied to clipboard

[API][v1] Remove poor use of `impl_poem.*` macros

Open banool opened this issue 3 years ago • 1 comments

In the Poem based API (v1) there are few places where I use the impl_poem_type and impl_poem_parameter macros. These macros are hacks to deal with types that I can't apply the regular derives from the Poem framework to (Object, Union, NewType, etc). impl_poem_type effectively erases the type of whatever it is applied to and treats it as a string, generally based on the JSON representation of that data.

Some of the reasons why we couldn't use the regular derives include:

  1. The type is outside of reach, e.g. in a crate in aptos-core that is too unrelated, or even worse, in a totally different crate (the move types are a great example of this). For these we need wrappers.
  2. The type is not expressible via OpenAPI. MoveType in api/types/src/move_types.rs is a great example of this, it is an enum in which some variants are empty while others have values. This is not allowed in OpenAPI, types must be either unions (variants with values) or enums (variants without values). For these we need to restructure the types.
  3. We would prefer to serialize the data differently than its standard representation. HexEncodedBytes is a good example of this. Internally, this is a Vec<u8>, but we know it is hex and prefer to represent it as a 0x string.

Only 3 is really a valid use case of these macros. A good general rule is if the type can be used with impl_poem_type but not with impl_poem_parameter (meaning it doesn't impl FromStr), it probably shouldn't be using these macros.

A quick audit suggests that the only types for which it's okay to use these macros are:

  • U64
  • U128
  • HexEncodedBytes

banool avatar Jul 28 '22 18:07 banool

The situation has improved a bit given some updates to the aptos-openapi crate (5200f1da52725d3fcd9d2581405c1bd9e1a0c71a), we can now represent types as objects, not just strings, attach helpful metadata, and use refs instead of inline refs.

banool avatar Jul 29 '22 01:07 banool

So the situation is pretty solid now, but having some more mainstream way to do this would be great, e.g. some official Poem derive.

banool avatar Aug 29 '22 23:08 banool

This issue is stale because it has been open 45 days with no activity. Remove the stale label or comment - otherwise this will be closed in 15 days.

github-actions[bot] avatar Nov 07 '22 06:11 github-actions[bot]