Completed Invoice Canister BNT-2 Bounty #465
Conversation
cannot view an already verified invoice
…eted unnecessary files
…l be fixed as additional task in bounty
…ication if paid but amount is less than transfer fee
…able amount works
…by aviate labs/icos
…d demonstrated working equivalence in unit tests
… as equivalence has already been demonstrated
…ee verification check instead of just reverting the code Originally I just reverted the code that was not working as intended as it broke tests related to other working functionality since I was going to introduce the fix as part of some other work to fix related issues, however it's simpler to just fix it now.
…changed references in unit tests
…eprocessing is necessary
…CP and ICRC1 types
|
Thanks @atengberg! I'll have a look later today. @krpeacock It would be great if you could have a look as well, since you have much more experience with the invoice canister. |
…s severe as documented, correctly updated docs
|
This is a doozy of a review and I'm just getting started, but I'm so happy that you've done such a good job with this project! |
| "candid": "src/token-ledger-canisters/icp/ledger.public.did", | ||
| "build": "" | ||
| }, | ||
| "icrc1_token_ledger_canister_ex1": { |
There was a problem hiding this comment.
Since we need a couple examples by default, I wonder if it would make sense to use ckBTC and SNS-1
There was a problem hiding this comment.
Re: earlier comment: Thanks, it was a great opportunity to get to know core aspects of and developing for the IC.
As far as ckBTC and SNS-1... at the time, the dfx sns install wasn't ready to be used along side dfx nns install, although it is what I originally wanted to do for an example of an ICRC1 token (using both the dfx nns and dfx sns commands in the main project). Additionally, because ckBTC/ckTESTBTC requires mainnet connection and local replicas can't connect to the mainnet (for now) , it didn't seem appropriate to add to them to the main project (although I do use the correct values for the ckTESTBTC token canister in the readme as an example of adding a supported token).
For both ckBTC and SNS-1 dedicated examples in the examples/ subdirectory could be more suitable to demonstrate aspects specific to either (particularly if at least mock ckBTC canisters could be deployed to make local replica development possible). This thought is finished in the next reply...
There was a problem hiding this comment.
Your reasoning makes sense!
I think that any ICRC-1 canister could be deployed as a "mock" ckBTC canister for the purposes of local development, apart from the minting and liquidating specifics. Someone may want a more fully fledged mock for those services in their application, but I definitely agree that it's beyond the scope of this repo
Simply by using an ICRC-1 canister like you are currently doing locally, and using the "remote" -> "id" config for those canisters in dfx.json would probably be sufficient, if we wanted to go that direction
| "type": "custom", | ||
| "wasm": "" | ||
| }, | ||
| "icp_ledger_canister": { |
There was a problem hiding this comment.
Why is icp_ledger different from nns-ledger?
There was a problem hiding this comment.
The two ICP ledger canisters references are different because one is for the included Rosetta wasm/did and the other for that of dfx nns install. There's a bit of story behind the actual end result of the choice of which token ledger canister(s) are deployed, and as far as how it works in the project it is explained in the beginning of clean-startup.mjs. I would have included those specifics in the readme as well, but it seemed like there was enough going on in the readme already.
There was a problem hiding this comment.
I think we can remove the included wasm. I only was using that originally because dfx nns install didn't exist yet at the time
There was a problem hiding this comment.
The reason I added it back after integrating dfx nns install was to provide an applied demonstration of the Local Ledger instructions as well as have another token in showing that SupportedToken type works for multiple tokens (along with the E2E tests). Also while dfx nns install can be used to deploy an ICP ledger canister, it might not always be feasible for a project because it comes as part of all the nns canisters that also must be installed (they are omitted from dfx.json as they are not needed, but they are all installed in the local replica as well when dfx nns install is used). Additionally, it gives an opportunity to show how zx can be used to automate swapping the private and public dids of the ICP ledger when deploying it with the wasm.
The motivation was to have the main project demonstrate the "main" ways an ICP (and ICRC1) ledger could be deployed while also "proving" the SupportedToken type can handle many entries. I had actually added both the Motoko references for the actor classes as well (for a total of 6 tokens), but that introduced some other issues, and having the wasms/dids of the Rosetta repository seemed to be the most relevant (along with dfx nns install) since they are intended (IUCC) for "external accountability" and designed for "plug n play" functionality.
If it doesn't make it too complicated, adding dfx sns install would then complete this idea particularly now that dfx sns install is working. Adding it along side dfx nns install would definitely be additional work that I thought could be a future task along with other features such as using Candb to autoscale invoice record storage, implementing CertifiedData or encryption with threshold ECDSA, adding (a rudimentary) "status" (variant) type to invoices to do things like proper refunding, making it possible to designate a consolidation address other than the creator's subaccount, providing JSON output to inbound HTTP calls, and/or refactoring the invoice canister functionality as independent module library components that could also be used elsewhere.
Another possibility is including a dedicated project in the examples/ subdirectory that could be tailored towards supporting a specific set of "mainnet ready" tokens (such as the mainnet ICP ledger with the ICRC1 standard as the base) that could be more readily used dropped into another project (and refactoring the Invoice Canister code to handle dynamic assignment of supported tokens/ledger canisters could be a part of that).
Although the ICP ledger wasm could be removed, I thought it'd be more helpful as an educational example to include it.
|
Apart from the github workflow which will probably just need one or two more changes to go green, this is looking really good! I've tested local deployment of the app and the examples, ran the tests, and I'm overall very happy with the quality of documentation and the many refinements you've added here |
|
Great! Thanks for taking the time to review it, hope it wasn't too much. I'm not sure how the github workflow works, but if there is something I can do to help let me know. Also, the bounty checklist is probably not necessary to be a part of the PR (though it'd still be apart of the commit history in case it might be useful in the future) if I should delete that or one of the reviewers wants to before the PR would actually be merged. Until the merge actually happens, I'll be sure to keep the branch synced with dfinity/examples. |
I'd leave the bounty checklist in! Maybe it can live in the As for the CI, you can click on the Details where there is a red X or visit the checks tab. In this case, https://github.com/dfinity/examples/actions/runs/4359939522/jobs/7663887717 shows that |
|
The bounty check list is moved to docs, and I added This was how the branch was originally configured, but I had dropped it due while integrating On another note, there wasn't a prompt to agree to the CLA bot for this PR although I had previously agreed for the #429 PR that was subsumed by this one. Since then I had updated my github username, and noticed there was another prompt to agree that has my correct username but references the previous PR. I definitely am not going to change my username again, but in case I also need to agree for this PR I thought I would mention this. On that note, can anyone confirm if I could delete that PR (it already has been closed) so as to keep things in this repository cleaner? |
|
Another note, it was not just a matter of adding @sesi200 I didn't want to modify the yaml in case there were other complications, does the networks configuration need to be removed after the e2e job finishes? Or should this be done in the start up script (I could check if there is a networks.json, backup if so, set the correct config, then set it back to the original / remove it if it was added)? In any case I updated the yaml, adding network.json into its own step prior to the running of e2e invoice step, and also adding a step to remove this networks.json after the e2e is completed as to not cause problems for other workflow examples. |
…including a corresponding step to remove it when finished
There was a problem hiding this comment.
the project uses a system wide networks.json file configured for the 8080 port and replica subnet type system
That's totally fine, as long as it is properly documented in the readme, which it is already.
I updated the e2e yaml to configure this accordingly, unless someone thinks it would be better as part of the startup script?
Adding it to the e2e yaml is my preferred way of solving it. I don't like it if a project modifies my networks.json without my consent.
does the networks configuration need to be removed after the e2e job finishes?
No, GH runners start from a clean slate for every test. You can screw up the system as much as you want and leave it in a terrible state, it will get deleted afterwards anyways.
|
@atengberg do you still have anything you'd like to or is it ready to merge from your perspective as well? |
This is the completed work for the grant-rfps bounty BNT-2: Invoice Canister.
The major change is support for ICRC1 token standard which is introduced through a type that allows for not only additional ICRC1 tokens but additional standards as well. Building on the motivation and design of the original invoice canister's implementation and keeping in mind the code is intended to be a part of the examples repository, there is extensive documentation and testing.
As there's a lot involved in the completion of this bounty, to make it easier to specifically review the bounty's requirements have been fulfilled, there's an included bounty task checklist outlining the tasks (and their subtasks) each with a summary explanation and mapped to relevant code files/lines (this could be removed before the PR would actually be merged).