Potential backwards incompatibility in ABI generated for sequence of pairs

[arhag]

Consider the ABIs generated by the following two fields in the core contract of EOS system contracts:
https://github.com/eosnetworkfoundation/eos-system-contracts/blob/a408ba6610f22ee6df312eb5e7a9897ee1522776/contracts/eosio.system/include/eosio.system/eosio.system.hpp#L443
https://github.com/eosnetworkfoundation/eos-system-contracts/blob/a408ba6610f22ee6df312eb5e7a9897ee1522776/contracts/eosio.system/include/eosio.system/eosio.system.hpp#L476

The first field (return_buckets) has type std::map<time_point_sec, int64_t> and the second (rex_maturities) has type std::deque<std::pair<time_point_sec, int64_t>>. While it would be preferable to just use std::vector in both cases, usage of these types in a table is acceptable and from the ABIs perspective they should both map to the same ABI types.

When compiling with CDT v3.0.0-rc3, the ABI generated for return_buckets is a list of a custom struct (pair_time_point_sec_int64[]) where the ABI for struct pair_time_point_sec_int64 is given by:

        {
            "name": "pair_time_point_sec_int64",
            "base": "",
            "fields": [
                {
                    "name": "key",
                    "type": "time_point_sec"
                },
                {
                    "name": "value",
                    "type": "int64"
                }
            ]
        },

When compiling with CDT v3.0.0-rc3, the ABI generated for rex_maturities has type B_pair_time_point_sec_int64_E[]) in which B_pair_time_point_sec_int64_E is a type alias that points to the same custom struct pair_time_point_sec_int64 defined above.

Some have reported that when compiling with the latest CDT 3.0.0, the ABI generated for rex_maturities now becomes pair_time_point_sec_int64[] directly (without the type alias intermediary) and return_buckets is still a list of the custom struct (pair_time_point_sec_int64[]), however, now the ABI definition of pair_time_point_sec_int64 has changed to:

        {
            "name": "pair_time_point_sec_int64",
            "base": "",
            "fields": [
                {
                    "name": "first",
                    "type": "time_point_sec"
                },
                {
                    "name": "second",
                    "type": "int64"
                }
            ]
        },

I was not personally able to replicate the above behavior exactly. When I compile EOS system contract with CDT 3.0.0, I still get the first version of the ABI.

However, I was able to get something close to the above difference by intentionally not including the rex_return_buckets (which holds the return_buckets field) in the ABI (by removing the [[eosio::table,eosio::contract("eosio.system")]] attribute from the struct). In this case, there is no ABI generated for the return_buckets field and the ABI generated for the rex_maturities field is still B_pair_time_point_sec_int64_E[]. But now the ABI for the custom struct pair_time_point_sec_int64 uses first and second as opposed to key and value.

The fact that CDT may generate an ABI for a sequence of pairs in different ways as described above can be considered a problem since the change of key to first and value to second (or vice versa) would be considered a breaking change to the interface of a smart contract.

This may be related to the changes of #23.

[linh2931]

I am not able to reproduce the problem for system contract either. But I could reproduce the problem @arhag observed using a simplified table:

...
         std::map<int32_t, int64_t> map_int32_int64;
         std::deque<std::pair<int32_t, int64_t>> deque_pair;
...

If I removed map_int32_int64 field, deque_pair's ABI was good. The pair would use first and second. I I kept map_int32_int64, the pair would use key and value, due to map was converted to a pair of key and value and deque_pair just used the pair definition introduced by the map. This is also confirmed if I moved map_int32_int64 after deque_pair, the pair would use first and second. Working on a solution.

[linh2931]

Post 3.0.0 rc-1 of mandel-cdt, nested containers and map were backported, which caused a couple of backward compatibility issues so far. #23 fixed one; here is the other one.

Following is a comparison between CDT rc-1 and 3.0.0

	CDT 3.0.0 rc-1	CDT 3.0.0 rc-2
std::map<int32_t, int64_t> my_map;	{ "name": "my_map", "type": "pair_int32_int64[]" }	{ "name": "my_map", "type": "pair_int32_int64[]" }, { "name": "pair_int32_int64", "base": "", "fields": [ {"name": "key", "type": "int32"}, { "name": "value", "type": "int64" } ] }
std::deque<std::pair<int32_t, int64_t>> deque_pair;	{ "name": "deque_pair" "type": "pair_int32_int64[]" }, { "name": "pair_int32_int64", "base": "", "fields": [ { "name": "first", "type": "int32" }, { "name": "second", "type": "int64" } ] }	{ "new_type_name": "B_pair_int32_int64_E", "type": "pair_int32_int64" }, {"name": "deque_pair","type": "B_pair_int32_int64_E[]"}, { "name": "pair_int32_int64", "base": "", "fields": [ { "name": "first", "type": "int32" }, { "name": "second", "type": "int64" } ] }
std::map<int32_t, int64_t> my_map; std::deque<std::pair<int32_t, int64_t>> deque_pair;	{ "name": "my_map", "type": "pair_int32_int64[]" },{ "name": "deque_pair" "type": "pair_int32_int64[]" }, { "name": "pair_int32_int64", "base": "", "fields": [ { "name": "first", "type": "int32" }, { "name": "second", "type": "int64" } ] }	{ "new_type_name": "B_pair_int32_int64_E", "type": "pair_int32_int64" }, { "name": "my_map", "type": "pair_int32_int64[]" },{ "name": "deque_pair" "type": "B_pair_int32_int64_E" }, { "name": "pair_int32_int64", "base": "", "fields": [ {"name": "key", "type": "int32"}, { "name": "value", "type": "int64" } ] }
std::deque<std::pair<int32_t, int64_t>> deque_pair; std::map<int32_t, int64_t> my_map;	{ "name": "deque_pair" "type": "pair_int32_int64[]" }, { "name": "my_map", "type": "pair_int32_int64[]" }, { "name": "pair_int32_int64", "base": "", "fields": [ { "name": "first", "type": "int32" }, { "name": "second", "type": "int64" } ] }	{ "new_type_name": "B_pair_int32_int64_E", "type": "pair_int32_int64" }, { "name": "deque_pair" "type": "B_pair_int32_int64_E" }, { "name": "my_map", "type": "pair_int32_int64[]" }, { "name": "pair_int32_int64", "base": "", "fields": [ {"name": "first", "type": "int32"}, { "name": "second", "type": "int64" } ] }

In rc-1, map was not fully supported as the pair type for map was not defined. Since rc-2, map is defined by a pair type with key and value fields. This causes problems when actual pairs with the same component types are used after a map, the map's pair type will be used.

Since map was just supported since rc-2, it is unlikely to be widely used. We can change the behavior without causing much trouble to the users. Two possible options:

1. Use first and second for the pair type defined for map. This removes the conflicts with regular pair type.
2. Use a different name for the type defined for map, like map_int32_int64, and keep key and value.

Option 1 is preferred, as the change is straightforward.

@arhag and @larryk85, which option do you prefer to?

[arhag]

I prefer option 1 as well.

So to clarify, with option 1, we basically go back to rc1 behavior except that std::map will actually be supported in the ABI and it will use the pair that has key names first and second?

It is unfortunate that it is a backwards incompatible change, but technically 3.0.0 introduced a backwards incompatible change to the ABI of std::deque as seen in row 3 (and which actually impacted the EOS system contracts). So I view this as a CDT bug fix to correct an unintentional backwards incompatible change that snuck its way into 3.0.0.

But please wait to get feedback from @larryk85 before working on any changes.

[larryk85]

Please use option 1.