Support new optional fields in transactions

[lmuntaner]

The issue raised in #230 comes from the Internet Computer Ledger application failing to decode a request with a new optional field.

In that case, the old request had the fields neuron_ids and include_neurons_readable_by_caller. When a new optional field include_empty_neurons_readable_by_caller was introduced, the error appeared.

We want to fix the list_neurons transaction and improve the Candid parser so that it ignores any new optional field as long as it's empty.

Optional field have the following type:

optionalField: [T] | []

Therefore, when a new optional field is passed with empty array: optionalField: [], the transaction should still work and the new field should be ignored.

I'm attaching test vectors for list_neurons to help testing the new development.

list-neurons.json.txt

There are different requests to list_neurons in this set of vectors. From requests without the new field include_empty_neurons_readable_by_caller to requests with the field empty or filled.

Let me know if you need anything else.

:link: zboto Link

[lmuntaner]

Cc: @dskloetd

[chcmedeiros]

Hey @lmuntaner ! I have some doubts regarding the test vectors. I see these two have the same blob for an include_empty_neurons_readable_by_caller not defined and defined

 "blob_candid": "d9d9f7a167636f6e74656e74a66361726758294449444c026d786c02acbe9cc50700dabcd1c70d7e01010200c8c056ea395dd500406c4830c8a17a006b63616e69737465725f69644a000000000000000101016e696e67726573735f6578706972791b17e48f440cf060006b6d6574686f645f6e616d656c6c6973745f6e6575726f6e736c726571756573745f747970656463616c6c6673656e646572581d19aa3d42c048dd7d14f0cfa0df69a1c1381780f6e9a137abaa6a82e302",
    "name": "List Neurons ",
    "candid_request": {
      "neuron_ids": ["15374508381553346560", "8836564053576663040"],
      "include_neurons_readable_by_caller": false,
      "include_empty_neurons_readable_by_caller": []
    },
    "output": [
      "0 | Transaction type : List Own Neurons",
      "1 | Neuron ID 1 : 15374508381553346560",
      "2 | Neuron ID 2 : 8836564053576663040"
    ]
  },
  {
    "blob_candid": "d9d9f7a167636f6e74656e74a66361726758294449444c026d786c02acbe9cc50700dabcd1c70d7e01010200c8c056ea395dd500406c4830c8a17a006b63616e69737465725f69644a000000000000000101016e696e67726573735f6578706972791b17e48f440cf060006b6d6574686f645f6e616d656c6c6973745f6e6575726f6e736c726571756573745f747970656463616c6c6673656e646572581d19aa3d42c048dd7d14f0cfa0df69a1c1381780f6e9a137abaa6a82e302",
    "name": "List Neurons ",
    "candid_request": {
      "neuron_ids": ["15374508381553346560", "8836564053576663040"],
      "include_neurons_readable_by_caller": false,
      "include_empty_neurons_readable_by_caller": [true]
    },
    "output": [
      "0 | Transaction type : List Own Neurons",
      "1 | Neuron ID 1 : 15374508381553346560",
      "2 | Neuron ID 2 : 8836564053576663040"
    ]
  },

Also when parsing the blob, I still get 2 elements when include_empty_neurons_readable_by_caller is defined, I was expecting 3 elements and the optional to be present, but it seems it isn't.

[lmuntaner]

Oh, thanks for catching that. That was my mistake @chcmedeiros

I uploaded the new test vectors.

Important note:

The vectors without include_empty_neurons_readable_by_caller are parsed with a different candid file. One that should not be used anymore. Therefore, I'm not sure if they are helpful for you. I added them to be exhaustive.

[lmuntaner]

I'd like to expand a bit more on the requirements and the context for this request.

Context:

• The list_neurons call is used in the NNS Dapp.
• A new optional field was added to list_neurons call in the backend.
• This new optional field broke the call to list_neurons of the IC application in the device.
• The team, changed the code in NNS Dapp to use the old candid file (without the new optional field) to solve the problem while the IC application was fixed.

Requirements:

1. IC application should support any unknown optional field as long as it's empty. If an unknown field is added in a payload and the value is null, then the application can ignore it and parse the rest.
2. IC application should support missing optional fields. If the candid payload doesn't have a specific field, as long as this field is optional, it can be considered as null and the IC application should continue with the action.
3. IC application should fail if an unknown (optional or not optional) field is passed with a value. If an unknown field is added in a payload and the value is NOT null, then the IC application should fail.

Example of 1: NNS dapp makes a call with include_empty_neurons_readable_by_caller = null and IC application doesn't know about this field. IC application should parse the rest and sign the payload.

Example of 2: NNS dapp makes a call without include_empty_neurons_readable_by_caller and IC application expects the optional field include_empty_neurons_readable_by_caller. IC application should sign the payload and consider the field as null.

Example of 3: NNS dapp makes a call with include_empty_neurons_readable_by_caller = true and IC application doesn't know about this field. IC application should fail.

Payload examples: I will be using didc to encode and decode Candid. Installation steps of didc.

We have a file test-candid.did with the following types:

type Foo = record {
  a: nat;
  b: opt nat;
};

type OldFoo = record {
  a: nat;
};

Payload of requirement 1:

$ didc encode -d ./scripts/test-vectors/test-candid.did -t '(Foo)' '(record { a = 10:nat; b = null })'
4449444c026c02617d62016e7d01000a00

IC application has OldFoo internally which doesn't know about b. It should continue.

Payload of requirement 2:

$ didc encode -d ./scripts/test-vectors/test-candid.did -t '(OldFoo)' '(record { a = 10:nat })'
4449444c016c01617d01000a

IC application has Foo internally which expects b optional field. It should continue.

Payload of requirement 3:

$ didc encode -d ./scripts/test-vectors/test-candid.did -t '(Foo)' '(record { a = 10:nat; b = opt 5})'
4449444c026c02617d62016e7d01000a0105

IC application has OldFoo internally which doesn't know about b. It should fail.

[chcmedeiros]

I will close the issue since version 3.0.3 added this support