Jordan's Review

[lastmjs]

I'll leave a review of the code in this repo here.

[lastmjs]

https://github.com/dacadeorg/icp-azle-201#ledger-canister

    "ledger_canister": {
      "type": "custom",
      "candid": "https://raw.githubusercontent.com/dfinity/ic/d87954601e4b22972899e9957e800406a0a6b929/rs/rosetta-api/icp_ledger/ledger.did",
      "wasm": "https://download.dfinity.systems/ic/d87954601e4b22972899e9957e800406a0a6b929/canisters/ledger-canister.wasm.gz",
      "remote": {
        "id": {
          "ic": "ryjl3-tyaaa-aaaaa-aaaba-cai"
        }
      }
    }

Is this the simplest way to define the pullable ledger canister? What about type: "pull"? This example seems to show a simpler way to do this: https://github.com/lwshang/pullable

I don't know much about dfx deps and pull yet, just pointing this out and wondering.

[lastmjs]

Watch out generally for typos like this in the README and elsewhere: " that is the canister that handles the authentication flow is deployed", that last part doesn't make sense gramatically.

[lastmjs]

Do not forget to run dfx generate dfinity_js_backend anytime you add/remove functions in the canister or when you change the signatures. Otherwise, these changes won't be reflected in IDL's and won't work when called using the JS agent.

Can you somehow do this automatically for the user, so that when the frontend is changed dfx generate is run? Seems that would be a better developer experience.

[lastmjs]

Why is azle a devDependency? It's part of the full application, not a devDependency

[lastmjs]

The frontend and backend package.json files have been conflated, and the frontend one has most of the complexity.

I think it might be better to put the package.json files into the frontend and backend directories respectively, and separate those concerns.

[lastmjs]

The dev dependencies seem to be conflated, anything that is actually used in the live application should be a dependency, things used for testing or developing only should be in devDependencies.

[lastmjs]

Also there seems to be an incredible amount of dependencies...can this not be made simpler?

[lastmjs]

https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L2

The ledger interfaces might not be completely up-to-date, if they're working against the latest icp legder Wasm then hopefully they are. Our examples/tests/that API needs to be looked into again. Just wanted to point that out, I imagine the worst case is you would get Candid errors.

[lastmjs]

I think it would be much more realistic to make ids uuids of some kind...like even just use the uuid package: https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L12

[lastmjs]

Product is not spelled correctly: https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L22

[lastmjs]

You might want to consider using variants and not enums: https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L51

I don't see why you wouldn't want to just use a variant in these cases, that's essentially what variants will do for you.

[lastmjs]

https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L51

I'm not sure that's the reasoning for using StableBTreeMap, you use StableBTreeMap for stable storage, otherwise you could just use other data structures on the heap that would probably be easier to deal with.

[lastmjs]

All of the max key and value stuff will go away once you upgrade to Azle 0.18.0 (which we are trying to get out the door right now).

[lastmjs]

I find it a bit strange that everything returns Response: https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L94 instead of giving each function a stricter return type.

[lastmjs]

I would recommend all functions returning Result<SomeType, SomeErrorType>

[lastmjs]

I really do not recommend doing this, use a uuid instead: https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L122

[lastmjs]

Use === or !== always as best practice: https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L119

[lastmjs]

Use const over let if you don't plan on mutating the variable or its value: https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L123

[lastmjs]

If seller is really a Principal then why is this text? https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L17

[lastmjs]

There isn't generally much authentication on these APIs...perhaps that's out of scope.

[lastmjs]

status should really be a variant not an enum: https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L33

[lastmjs]

As a best practice I would not mutate things like this: https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L147

Generate the response (though I don't think you should have one response object that every method returns, perhaps) at once, in each branch. If you have conditional logic then you can create a function that will return a Response based on the logic.

[lastmjs]

I would stay away from mutations like this: https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L194

[lastmjs]

The structure of this seems strange: https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L82

I would consider using Principal, but also consider organizing your StableBTreeMaps more like database tables. Each one should have keys as primary keys with their values. You can then use the primary keys and store them inside of the values to create relationships....

But in this case maybe my main issue is string should be principal...just some thoughts, I haven't thought about it as deeply as you.

[tarasherasymchuk]

Watch out generally for typos like this in the README and elsewhere: " that is the canister that handles the authentication flow is deployed", that last part doesn't make sense dramatically.

I will have a look at the typos. However, this README file is not intended to be used by the end-users, it's just a set of instructions to use internally. The final repo with the course materials won't use this file directly.

Anyway, thanks for pointing out it.

[tarasherasymchuk]

Also there seems to be an incredible amount of dependencies...can this not be made simpler?

actually, yes. majority of dependencies are required for the react app to run as a canister

[tarasherasymchuk]

You might want to consider using variants and not enums: https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L51

I don't see why you wouldn't want to just use a variant in these cases, that's essentially what variants will do for you.

Used enums there to reuse error messages. It looks cleaner to me:

enum Message {
    NotFound = "PRODUCT_NOT_FOUND",
    InvalidPayload = "INVALID_PAYLOAD",
    PaymentFailed = "PAYMENT_FAILED",
    PaymentCompleted = "PAYMENT_COMPLETED",
};

// ...

response = { error: Message.NotFound }

instead of:

type Message = Variant<{
    NotFound: string;
    InvalidPayload: string;
    PaymentFailed: string;
    PaymentCompleted: string;
}>;

// ...

response = { error: {NotFound: "PRODUCT_NOT_FOUND"} }

However, it might be helpful when we enrich error messages with some additional data depending on the context but in this particular case, we have simple error messages that are re-used.

[tarasherasymchuk]

https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L51

I'm not sure that's the reasoning for using StableBTreeMap, you use StableBTreeMap for stable storage, otherwise you could just use other data structures on the heap that would probably be easier to deal with.

The main reason for using StableBTreeMap is to keep data after canister upgrades

[tarasherasymchuk]

There isn't generally much authentication on these APIs...perhaps that's out of scope.

There was no intention to build the auth API. We only need to authenticate in a browser using the internet identity canister to make transactions and to have the right context in the ic.*

[tarasherasymchuk]

The structure of this seems strange: https://github.com/dacadeorg/icp-azle-201/blob/main/src/dfinity_js_backend/src/index.ts#L82

I would consider using Principal, but also consider organizing your StableBTreeMaps more like database tables. Each one should have keys as primary keys with their values. You can then use the primary keys and store them inside of the values to create relationships....

But in this case maybe my main issue is string should be principal...just some thoughts, I haven't thought about it as deeply as you.

Agree about the string type for Principal, will change it to Principal.

Regarding the structure where we store an array of orders per principal: initially, I'd considered implementing a method to buy multiple products per transaction. But later changed to a single product per order so it does not make any sense to keep it like that. Will change it to Order.

[tarasherasymchuk]

https://github.com/dacadeorg/icp-azle-201#ledger-canister

    "ledger_canister": {
      "type": "custom",
      "candid": "https://raw.githubusercontent.com/dfinity/ic/d87954601e4b22972899e9957e800406a0a6b929/rs/rosetta-api/icp_ledger/ledger.did",
      "wasm": "https://download.dfinity.systems/ic/d87954601e4b22972899e9957e800406a0a6b929/canisters/ledger-canister.wasm.gz",
      "remote": {
        "id": {
          "ic": "ryjl3-tyaaa-aaaaa-aaaba-cai"
        }
      }
    }

Is this the simplest way to define the pullable ledger canister? What about type: "pull"? This example seems to show a simpler way to do this: https://github.com/lwshang/pullable

I don't know much about dfx deps and pull yet, just pointing this out and wondering.

If I understand correctly, that's the ledger canister that must be configured as pullable and only then we can pull it as a dependency via a static id. I guess we can the current config of how Ledger canister is pulled and deployed locally

[tarasherasymchuk]

The frontend and backend package.json files have been conflated, and the frontend one has most of the complexity.

I think it might be better to put the package.json files into the frontend and backend directories respectively, and separate those concerns.

It's a great catch, thanks. However, this project structure is generated by dfx and we advise using it in the course so I guess we should keep the structure of the project as-is and not enforce any structural changes as it's out of the scope of the course.

[tarasherasymchuk]

Do not forget to run dfx generate dfinity_js_backend anytime you add/remove functions in the canister or when you change the signatures. Otherwise, these changes won't be reflected in IDL's and won't work when called using the JS agent.

Can you somehow do this automatically for the user, so that when the frontend is changed dfx generate is run? Seems that would be a better developer experience.

The only thing we can do is to provide an npm script, like, gen-deploy:local where we would generate and deploy the backend canister. Any other more complex solution that automates this process goes beyond the scope of the course.