dfinity / invoice-canister

Providing an example and simplified experience for accepting payments in smart contracts
Apache License 2.0
44 stars 13 forks source link

Invoice canister: code review #3

Closed crusso closed 2 years ago

crusso commented 2 years ago

Mostly superficial suggestions to invoice canister code (didn't touch the examples, though similar refactorings probably apply).

The did file looks more complicated than necessary. You seem to name the argument and return types for each function of the service - since these types are referenced exactly once, would it not be easier to just inline them in the function signatures, or is this a trick used to make Rust interop easier somehow. Inlining the argument and result type definitions might actually make the Candid easier to read from a user perspective.

It also wasn't clear to me which of the code is new, and which is copied down libraries. Is there any reason not to use vessel to pull down the packages for Hex coding, SHAxxx etc? Did you actually edit the code for these? There would be less code to review and less chance of deviation by missing fixes to the packages themselves.

I did the cleanups as a sequence of commits, so you can pick and choose the one you want (if any).

crusso commented 2 years ago

@crusso remove redundant array copy

Should read remove redundant record copy.

krpeacock commented 2 years ago

The choice to name the arguments and return types does sacrifice readability, but it is consistent, and I believe it will be safer from an upgradeability perspective, since argument counts will never change.

For the libraries - I have modified Hex to use lowercase encodings, but CRC32 and SHA224 can be pulled in as libraries