Skip to content

Conversation

@crusso
Copy link
Contributor

@crusso crusso commented Mar 17, 2025

Description

Bring Candid decoding of opt types up to Candid spec:

In particular, when decoding at an opt type:

  • If the wire type is an opt type, decode its payload at the expected content type
    (as before).
  • Allow decoding null wire type as IDL value null (i.e. JS []).
  • Allow decoding of value of reserved wire type, defaulting to IDL value null (i.e. JS []).
  • Allow decoding of wider variant type on the wire at narrower expected variant type,
    provided the decoded value is valid at the expected variant type. Otherwise, default to null (i.e. JS []).
  • Otherwise:
    • If the expected content type is null or reserved or (nested) opt, return IDL value null (i.e. JS []).
    • The expected content type is neither null, reserved or nested opt:
      allow decoding of the non-optioned value v as opt v (JS [v*]) if compatible with
      the expected content type; if incompatible, return IDL value null (JS []).

How Has This Been Tested?

See additional candid tests in PR.

Checklist:

  • My changes follow the guidelines in CONTRIBUTING.md.
  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation (no real change - c.f. current candid spec)

@crusso crusso changed the title experiment: decoding variants - remaining edge cases experiment: decoding options - remaining edge cases Mar 17, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Mar 17, 2025

size-limit report 📦

Path Size
@dfinity/agent 72.08 KB (+0.11% 🔺)
@dfinity/candid 11.82 KB (+1.04% 🔺)
@dfinity/principal 4.21 KB (0%)
@dfinity/auth-client 50.17 KB (+0.17% 🔺)
@dfinity/assets 67.88 KB (+0.06% 🔺)
@dfinity/identity 47.73 KB (+0.07% 🔺)
@dfinity/identity-secp256k1 106.82 KB (+0.22% 🔺)

@crusso crusso changed the title experiment: decoding options - remaining edge cases fix: decoding options - remaining edge cases Mar 17, 2025
@crusso crusso changed the base branch from main to claudio/variant-fix March 17, 2025 20:32
@crusso crusso marked this pull request as ready for review March 18, 2025 01:24
@crusso crusso requested a review from a team as a code owner March 18, 2025 01:24
@crusso crusso mentioned this pull request Mar 18, 2025
4 tasks
@crusso
Copy link
Contributor Author

crusso commented Mar 18, 2025

FTR, this is how I hand crafted the tests using Motoko, in case we want to add more:

import Text "mo:base/Text";
import Char "mo:base/Char";

actor Echo {
 
  func esc(b:Blob): Text {
    let t = Text.toLowercase(debug_show b);
    Text.translate(t, func c { if (c == '\\') "" else Char.toText(c)})
  };

  type A1 = {a : ?{#x; #y}};

  type A2 = {a : ?{#x; #y; #z}};

  type R1 = {a : {#x; #y}};

  type R2 = {a : {#x; #y; #z}};

  type Anull = {a : Null};

  type Aany = {a : Any};

  public query func test_z() : async Text {
    let b2 = to_candid ({a = ?#z} : A2);
    let ?a : ?A1 = from_candid b2;
    assert a == {a = null};
    debug_show {enc = esc(b2); dec = a}
  };

  public query func test_x() : async Text {
    let b2 = to_candid ({a = ?#x} : A2);
    let ?a : ?A1 = from_candid b2;
    assert a == {a = ?#x};
    debug_show {enc = esc(b2); dec = a}
  };

  public query func test_null() : async Text {
    let b2 = to_candid ({a = null} : Anull);
    let ?a : ?A1 = from_candid b2;
    assert a == {a = null};
    debug_show {enc = esc(b2); dec = a}
  };

  public query func test_any() : async Text {
    let b2 = to_candid ({a = ()} : Aany);
    let ?a : ?A1 = from_candid b2;
    assert a == {a = null};
    debug_show {enc = esc(b2); dec = a}
  };
  
  public query func test_non_opt() : async Text {
    let b2 = to_candid ({a = #x} : R1);
    let ?a : ?A1 = from_candid b2;
    assert a == {a = ?#x};
    debug_show {enc = esc(b2); dec = a}
  };

  public query func test_non_opt_wider_expected() : async Text {
    let b2 = to_candid ({a = #x} : R2);
    let ?a : ?A1 = from_candid b2;
    assert a == {a = ?#x};
    debug_show {enc = esc(b2); dec = a}
  };

  public query func test_non_opt_wider_unexpected() : async Text {
    let b2 = to_candid ({a = #z} : R2);
    let ?a : ?A1 = from_candid b2;
    assert a == {a = null};
    debug_show {enc = esc(b2); dec = a}
  };


  public query func test_non_opt_other() : async Text {
    let b2 = to_candid ({a = 1} : {a:Nat});
    let ?a : ?A1 = from_candid b2;
    assert a == {a = null};
    debug_show {enc = esc(b2); dec = a}
  };

   public query func test_expect_opt_null() : async Text {
    let b2 = to_candid ({a = 1} : {a:Nat});
    let ?a : ?{a:?Null} = from_candid b2;
    assert a == {a = null};
    debug_show {enc = esc(b2); dec = a}
  };
   public query func test_expect_opt_opt() : async Text {
    let b2 = to_candid ({a = 1} : {a:Nat});
    let ?a : ?{a : ??Nat} = from_candid b2;
    assert a == {a = null};
    debug_show {enc = esc(b2); dec = a}
  };
   public query func test_expect_opt_any() : async Text {
    let b2 = to_candid ({a = 1} : {a:Nat});
    let ?a : ?{a :?Any}= from_candid b2;
    assert a == {a = null};
    debug_show {enc = esc(b2); /* dec = a */}
  };
}


@crusso
Copy link
Contributor Author

crusso commented Mar 18, 2025

More tests that check decoding of embedded options with more to decode following:

import Text "mo:base/Text";
import Char "mo:base/Char";

actor Echo {
 
  func esc(b:Blob): Text {
    let t = Text.toLowercase(debug_show b);
    Text.translate(t, func c { if (c == '\\') "" else Char.toText(c)})
  };

  type A1 = {a : ?{#x; #y}; b : Text};

  type A2 = {a : ?{#x; #y; #z}; b : Text};

  type R1 = {a : {#x; #y}; b : Text};

  type R2 = {a : {#x; #y; #z}; b : Text};

  type Anull = {a : Null; b : Text};

  type Aany = {a : Any; b: Text};

  public query func test_z() : async Text {
    let b2 = to_candid ({a = ?#z; b = "abc"} : A2);
    let ?a : ?A1 = from_candid b2;
    assert a == {a = null; b = "abc"};
    debug_show {enc = esc(b2); dec = a}
  };

  public query func test_x() : async Text {
    let b2 = to_candid ({a = ?#x; b =  "abc"} : A2);
    let ?a : ?A1 = from_candid b2;
    assert a == {a = ?#x; b = "abc"};
    debug_show {enc = esc(b2); dec = a}
  };

  public query func test_null() : async Text {
    let b2 = to_candid ({a = null; b = "abc"} : Anull);
    let ?a : ?A1 = from_candid b2;
    assert a == {a = null; b = "abc"};
    debug_show {enc = esc(b2); dec = a}
  };

  public query func test_any() : async Text {
    let b2 = to_candid ({a = (); b = "abc"} : Aany);
    let ?a : ?A1 = from_candid b2;
    assert a == {a = null; b = "abc"};
    debug_show {enc = esc(b2); dec = a}
  };
  
  public query func test_non_opt() : async Text {
    let b2 = to_candid ({a = #x; b = "abc"} : R1);
    let ?a : ?A1 = from_candid b2;
    assert a == {a = ?#x; b = "abc"};
    debug_show {enc = esc(b2); dec = a}
  };

  public query func test_non_opt_wider_expected() : async Text {
    let b2 = to_candid ({a = #x; b = "abc"} : R2);
    let ?a : ?A1 = from_candid b2;
    assert a == {a = ?#x; b = "abc"};
    debug_show {enc = esc(b2); dec = a}
  };

  public query func test_non_opt_wider_unexpected() : async Text {
    let b2 = to_candid ({a = #z; b="abc"} : R2);
    let ?a : ?A1 = from_candid b2;
    assert a == {a = null; b = "abc"};
    debug_show {enc = esc(b2); dec = a}
  };


  public query func test_non_opt_other() : async Text {
    let b2 = to_candid ({a = 1; b = "abc"} : {a:Nat; b: Text});
    let ?a : ?A1 = from_candid b2;
    assert a == {a = null; b = "abc"};
    debug_show {enc = esc(b2); dec = a}
  };


}

@crusso crusso changed the title fix: decoding options - remaining edge cases fix: make IDL/Candid decoding of options spec compliant Mar 18, 2025
@crusso crusso changed the base branch from claudio/variant-fix to main March 18, 2025 23:21
@crusso crusso changed the base branch from main to claudio/variant-fix March 18, 2025 23:46
@crusso crusso changed the base branch from claudio/variant-fix to main March 19, 2025 00:38
@crusso crusso requested a review from krpeacock March 19, 2025 00:38
@krpeacock krpeacock closed this Mar 21, 2025
@krpeacock krpeacock reopened this Mar 21, 2025
@krpeacock krpeacock merged commit b623f0d into main Mar 21, 2025
112 of 118 checks passed
@krpeacock krpeacock deleted the claudio/variant-edge-cases branch March 21, 2025 19:29
github-merge-queue bot pushed a commit to dfinity/nns-dapp that referenced this pull request Mar 25, 2025
# Motivation

1. Keep our dependencies up to date.
2. A bug in [agent-js](dfinity/icp-js-core#981)
that caused issues decoding variants from candid

# Changes

1. Ran `npm i @dfinity/agent@latest`.

# Tests

1. Existing tests pass.

# Todos

- [ ] Add entry to changelog (if necessary).
not necessary
@crusso
Copy link
Contributor Author

crusso commented Mar 28, 2025

Hurrah!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants