This is a list of all the tasks specified as part of the bounty to be completed before submission for approval.
They can be originally found at BNT-2: Invoice Canister #2.
This was created to make it easier to know what needed to be done as to verify everything has been completed.
ㅤ Generally for every operation involving an ICP address or transaction there needs to be an equally working operation for an ICRC1 address or transaction.
-
Incorporate the actual ICRC1 token-ledger canister(s).
✶ With the development ofSupportedToken
to demonstrate it works, two ICRC1 token-canister ledgers are installed and deployed from a downloaded wasm and did of the offical Dfinity Rosetta repository for ICRC1.-
src/token-ledger-canisters/icrc1/
(icrc1.did
&ledger.wasm
) (template shell script also provided). - Updated
dfx.json
icrc1_token_ledger_canister_ex1
&..._ex2
(lines 29-40). - Update the install script to deploy both and prepare for E2E testing.
-
clean-spinup.mjs::deploy_icrc1_token_canister()
(lines 189-214, 370-374). -
clean-spinup.mjs::disburse_funds_to_nnsFundedSecp256k1Identity_creator_subaccounts()
(lines 250-270, 377).
-
-
-
Adding the required ICRC1 token-ledger canister typings:
✶src/invoice/modules/SupportedToken.mo::TokenSpecific.ICRC1
(lines 197-235). -
For consistency and integration adding the ICRC1 token-ledger canister actor supertype typing:
✶src/invoice/modules/SupportedToken.mo::TokenSpecific.ICRC1.Supertype
(lines 238-243). -
Adding the logic for ICRC1 addressing computations:
-
src/invoice/modules/SupportedToken.mo::TokenSpecific.ICRC1.Adapter
(lines 246-494).-
isValidSubaccount()
-
isValidAddress()
-
encodeAddress()
-
decodeAddress()
-
computeInvoiceSubaccount()
-
computeInvoiceSubaccountAddress()
-
computeCreatorSubaccount()
-
computeCreatorSubaccountAddress()
-
- Each has at least one unit test in
test/unit/Test.mo
✶describe("ICRC1 Adapter Account and Subaccount Computations"...
(lines 268-420).
-
-
Adding the logic connecting those addressing compuations for use of by the invoice canister's methods:
-
src/invoice/modules/SupportedToken.mo
(lines 525-975).
ㅤㅤㅤRelatedSupportedToken
's fields:-
SupportedToken<T1, T2>
(#ICRC1_ExampleToken
&#ICRC1_ExampleToken2
(lines 528-529). -
UnitType = SupportedToken<(), ()>
-
Amount = SupportedToken<TokenSpecific.ICP.Tokens, TokenSpecific.ICRC1.Tokens>
-
Address = SupportedToken<TokenSpecific.ICP.AccountIdentifier, TokenSpecific.ICRC1.Account>
-
TransferArgs = SupportedToken<TokenSpecific.ICP.TransferArgs, TokenSpecific.ICRC1.TransferArgs>
-
TransferResult = SupportedToken<TokenSpecific.ICP.TransferResult, TokenSpecific.ICRC1.TransferResult>
-
TransferSuccess = SupportedToken<TokenSpecific.ICP.BlockIndex, TokenSpecific.ICRC1.TxIndex>
-
TransferErr = SupportedToken<TokenSpecific.ICP.TransferError, TokenSpecific.ICRC1.TransferError>
ㅤRelatedSupportedToken
's methods each with corresponding ICRC1 cases: -
getTransactionFee()
-
unwrapTokenAmount()
-
wrapAsTokenAmount()
-
getTokenVerbose()
-
encodeAddress()
-
encodeAddressOrUnitErr()
-
getAddressOrUnitErr()
-
getInvoiceSubaccountAddress()
-
getEncodedInvoiceSubaccountAddress()
-
getCreatorSubaccountAddress()
-
getTransferArgsFromInvoiceSubaccount()
-
getTransferArgsFromCreatorSubaccount()
-
rewrapTransferResults()
-
getDefaultSubaccountAddress()
-
-
Unit testing for each of the above methods in
test/unit/Test.mo
.
ㅤEach includes its own subsuite-set of test cases ("describe">"it") for each token type.-
describe("Supported Token Types' and Methods"...
(full set omitted here, lines 421-1651).
-
-
Implementing the actual use of the above methods in the invoice canister's API methods in
src/invoice/Invoice.mo
:-
create_invoice()
-
get_caller_address()
-
get_caller_balance()
-
get_invoice()
-
verify_invoice()
-
transfer()
-
recover_invoice_subaccount_balance()
-
to_other_address_format()
-
-
E2E testing for each above the methods in
test/e2e/src/tests/
:
ㅤEach includes its own subsuite-set of test cases ("describe">"it") for each token type and other test case conditions where appropiate.-
create_invoice.test.js
-
get_caller_address.test.js
-
get_caller_balance.test.js
-
get_invoice.test.js
-
verify_invoice.test.js
-
transfer.test.js
-
recover_invoice_subaccount_balance.test.js
-
to_other_address_format.test.js
-
-
-
Updating
motoko-seller-client
project to demonstrate seller flow also integrating ICRC1 compatable invoice canister:
ㅤProject scope change: instead of four, only two two tokens and correspondingSupportedToken
variant tags are used:#ICP
and#ICRC1
.-
Adding mock token-canister ledgers1.
✶examples/motoko-seller-client/src/backend/modules/MockTokenLedgerCanisters.mo...
ㅤShould correctly return every #Ok/#Err result of balance/transfer except ICRC1's Generic/TempUnavailable Err.-
...ICP.MockLedger
(lines 96-238; also withdeposity_free_money
). -
...ICRC1.MockLedger
(lines 96-238; also withdeposity_free_money
).
-
-
Updating the backend
- Updating
examples/motoko-seller-client/src/backend/modules/SupportedToken.mo
:- For each corresponding methods (and
SupportedToken<>
variant) two cases instead of the previous four.
- For each corresponding methods (and
- Updating
examples/motoko-seller-client/src/backend/modules/SupportedToken.mo
:- Editing to do
#ICP
and#ICRC1
instead of four cases for each method.
- Editing to do
- Updating
examples/motoko-seller-client/src/backend/Invoice.mo
:- Editing to do two instead of four cases for each API method.
- Adding the seller as an authorized allowed creator.
- Updating the
deposit_free_money
logic to handle both ICP and ICRC1 token types.
- Updatting
examples/motoko-seller-client/src/backend/Seller.mo
:- Correctly importing
Invoice
canister now that it's a class actor. - Updating each method to still do expected functionality.
- Correctly importing
- Updating
-
Updating the frontend
✶examples/motoko-seller-client/src/frontend/...
:- Adding the needed known identities
.../src/identity.js
to correctly create actor types. - Updating
.../src/components/Invoice.jsx
:- to handle accepting both ICP and ICRC1 token types (lines 68-80, 98).
- to display payment address correctly (line 106).
- Updating
.../src/components/InvoicePayDialog.jsx
: (lines 68, 71).- to allow selection of payment token type (lines 14, 15, 17-24, 28-39, 43-50).
- Updating
.../src/components/InvoiceManager.jsx
:- to handle creating, getting both token types (lines 19-36).
- fixed bug showing previous invoice while new one is being created between displaying different invoices (lines 13-17, 47).
- Updating
.../src/components/Payment.jsx
:- to process initiating payment for either token type (lines 10-18, 22-25).
- Adding the needed known identities
-
- dfinity/examples#292
ㅤPre-existing issues that remained to be resolved. In particular:- Add access control for creating new invoices (see [SEC-F20] & [SEC-F21] below).
- Refactor permission checks to a method.
✶src/invoice/Invoice.mo
line 98 (getInvoiceIfAuthorized
). - Additionally, when first starting this bounty independently of any work I was doing, the startup scripting was being migrated to use zx which coincidentally at the time I had just become interested in. There's likely an ideal niche for dfx cli and zx for example in making dynamic canister deployment easier particularly as the javascript can console log out the arg as a literal without the explicit need of using it with zx. In any case as a result this migration was completed in the form of
clean-spinup.mjs
.
-
dfinity/invoice-canister#35
ㅤThis became a two part issue with a follow up. With the added refund logic, there are three scenarios when the invoice canister calls the transfer logic of a token-ledger canister and before that call can take place it can result in an arithmetic overflow or underflow (if it isNat64
orNat
)2 when the transfer fee is subtracted from the amount to transfer. These three scenarios are:- User calls to transfer a specific amount from their creator subaccount.
- User calls for a partial refund or recovery of missent funds from an invoice subaccount.
- During the normal life cycle of an invoice upon successful verification of payment when the proceeds are transferred from the invoice's subaccount to the subaccount of that invoice's creator.
ㅤTo account for the first two cases the
#err kind #InsufficientTransferAmount
is added; to account for the third#err kind #InsufficientAmountDue
is added since proceeds of invoices with an amount due less than the transfer fee are effectively irrecoverable3 if each invoice has it's own subaccount as a payment address. Although ICRC1 token-ledger canisters can handle the fee automatically (as an opt transfer arg), it was easier to normalize preventing the error than handling its return which, in addition to also including the necessary support for ICP ledgers, provides a more uniform API for the user.
ㅤAs this issue is specifically resolved in the code:-
create_invoice()
:
✶src/Invoice/modules/Types.mo
line 187 (#InsufficientAmountDue;
).
✶src/Invoice/Invoice.mo
(line 202).
✶test/e2e/src/tests/create_invoice.test.js
(lines 520-549 all four token types tested). -
transfer()
:
✶src/Invoice/modules/Types.mo
(line 373:#InsufficientTransferAmount;
).
✶src/Invoice/Invoice.mo
(line 541).
✶test/e2e/src/tests/transfer.test.js
(lines 308-356, all four token types tested). -
recover_invoice_subaccount_balance()
:
✶src/Invoice/modules/Types.mo
(line 433:#InsufficientTransferAmount;
).
✶src/Invoice/Invoice.mo
(line 672).
✶test/e2e/src/tests/transfer.test.js
:
ㅤ✶ lines 313-349 (#ICP).
ㅤ✶ lines 539-575 (#ICP_nns).
ㅤ✶ lines 769-805 (#ICRC1_ExampleToken).
ㅤ✶ lines 1004-1041 (#ICRC1_ExampleToken2).
- dfinity/invoice-canister#28
For both ICP and ICRC1 computed subaccounts:- ICP:
✶src/invoice/modules/SupportedToken.mo::TokenSpecific.ICP.Adapter.computeCreatorSubaccount()
(line 157).
ㅤㅤ↳was previousilysrc/invoice/Account.mo::principalToSubaccount()
- ICRC1:
✶src/invoice/modules/SupportedToken.mo::TokenSpecific.ICRC1.Adapter.computeCreatorSubaccount()
(line 335).
- ICP:
-
dfinity/invoice-canister#25
ㅤRepresented by the logic of three different methods, depending on where the check is occuring; all the canister's API methods checks against the anonymous principal by calling at least one of the following:- Preventing the canister installer from adding an allowed creator as the anonymous principal.
✶src/invoice/Invoice.mo
(line 237 andsrc/invoice/modules/Types.mo
line 90).
ㅤ(which in turn prevents the anonymous principal from calling any other method, specifically because checks done in all the other methods by one or the other of the following two then prevent) - Unauthorized calls by principals not on allowed creators list when the call is not invoice specific.
✶src/invoice/Invoice.mo::hasCallPermission_()
(line 91).
ㅤㅤ(and if it is)
✶src/invoice/Invoice.mo::getInvoiceIfAuthorized_()
(lines 98-135).
ㅤㅤ(which uses the previoushasCallPermission_()
method)
✶
test/e2e/src/tests/disallowAnonymous.test.js
(entire file) demonstrates verified coverage for each API method. - Preventing the canister installer from adding an allowed creator as the anonymous principal.
- dfinity/invoice-canister#21
ㅤThis does not have an explicit test at this time. That being said, a means to resolving this issue is implemented by a lock synchronizing access of an invoice by its id when either theverify_invoice
orrecover_invoice_subaccount_balance
is called. In turn either method could trigger a transfer from that invoice's subaccount which with ungaurded concurrent access could lead to problems as discussed in that issue (and potentially more with the added recovery of funds functionality).
ㅤTo ensure the lock itself does not become a problem, it is implemented with an auto-expiring timeout; all inter-canister calls are wrapped with a try/catch; other code in the scope of the lock has been tested and accounted for (preventing trapping from subtracting amounts less than a transfer fee, for example). This means of resolution was brought up on the forums as well, and this approach given tentantive approval (the timeout may itself prevent problems, but if either method takes longer than ten minutes--the currently set expiration time--either method would likely need to be called again); while there might be a better built-in solution available for Motoko, it is not yet available.
ㅤAs invoices are already access controlled (only callers on verify permission list could call either method for a given invoice) and are only linked as a sender from their own subaccount, this issue is even more of an edge case. The auto-expiring lock prevents it and other potential issues from concurrent calls to verify and recover causing problems. To see the specific code:
✶src/invoice/Invoice.mo
(map and timeout declarations line 75 & 76).
✶src/invoice/Invoice.mo::verify_invoice
(each branch covered lines 390-508).
✶src/invoice/Invoice.mo::recover_invoice_subaccount_balance()
(each branch covered lines 631-722).
-
dfinity/invoice-canister#20
ㅤUpgrading the invoice canister to use libraries as opposed to hard-coded copying of sha256, crc32, and hex libraries. This is done by adding Aviate Labs Internet Computer Open Services package set as an additional upstream inpackage-set.dhall
to make the"array", "crypto", "hash", "encoding", "principal"
dependencies available invessel.dhall
. The existing addressing computations for account identifiers is updated as well as 1-1 Motoko unit tests with existing tests to show equivalence between the two implementations (added a tag to jump to that commit which is no longer a part of visible code base)4. Most of those same methods as they are now:-
src/invoice/modules/SupportedToken.mo::TokenSpecific.ICP.Adapter.computeInvoiceSubaccount
(lines 121-136).
ㅤ↳was previousilysrc/invoice/Utils.mo::generateInvoiceSubaccount()
-
src/invoice/modules/SupportedToken.mo::TokenSpecific.ICP.Adapter.computeCreatorSubaccount
(lines 152-164).
ㅤ↳was previousilysrc/invoice/Account.mo::principalToSubaccount()
-
src/invoice/modules/SupportedToken.mo::computeInvoiceSubaccountAddress
(lines 140-144).
ㅤ↳was previousilysrc/invoice/Account.mo::accountIdentifier()
-
src/invoice/modules/SupportedToken.mo::computeCreatorSubaccountAddress
(lines 168-).
ㅤ↳was previousilysrc/invoice/Account.mo::accountIdentifier()
-
src/invoice/modules/SupportedToken.mo::TokenSpecific.ICP.Adapter.isValidAddress
) (lines 94-).
ㅤ↳was previousilysrc/invoice/Account.mo::validateAccountIdentifier()
ㅤNote that the
ICRC1.Adapter
module also uses these libraries for itscomputeCreatorSubaccount
method.
There's also coverage of the entireICP.Adapter
module (same asICRC1.Adapter
above) using these library dependencies at:-
test/unit/Test.mo
(lines 107-267).
ㅤ✶describe("ICP Adapter AccountIdentifier and Subaccount Computations")
(entire set of tests omitted).
-
-
dfinity/invoice-canister#16
ㅤWhile adding theCertifiedMap
library is a potential option for future development, all the calls have been made update calls (as well as discussion regarding why in the non-generated developer docs):-
src/Invoice.mo::get_invoice()
(line 292). -
src/Invoice.mo::get_caller_balance()
(line 311, previousilyget_balance
). -
src/Invoice.mo::get_caller_address()
(line 292, previousilyget_account_identifier
). -
src/Invoice.mo::to_other_address_format()
(line 750).
-
- dfinity/invoice-canister#13
ㅤNow any amount more than the transfer fee cost can be transferred out of an invoice subaccount by its creator or those on its verify permission list. If the invoice has not yet been verified, this could serve as a refund; if the invoice has already been verified, this method can be used to recover those funds. It should be noted this is not a refund for invoices already verified as those funds are moved to the creator's subaccount when the balance paid is confirmed to be greater or equal to the invoice's amount due. Since the verification and balance recovery methods are synchronized by invoice id, a call to recover funds will only happen after the invoice verification is complete (see [SEC-F05] above). Therecover_invoice_subaccount_balance
also has extensive E2E testing.-
src/Invoice.mo::recover_invoice_subaccount_balance()
(lines 613-738). -
src/modules/Types.mo::recover_invoice_subaccount_balance
(lines 399-441). -
test/e2e/src/tests/recover_invoice_subaccount_balance.test.js.mo
ㅤ✶ (entire file, each token has its own test subsuite, in addition to non-token specific tests).
-
-
dfinity/invoice-canister#12
ㅤImplementating access control can get very complicated, particularly if the objective is to use the invoice canister for managing the finances of a DAO. While not impossible, it is beyond the scope of this bounty. That being said an allowed creators list is added such that the original invoice canister installer has the unique right to add or remove principals from the allowed creators list. It should be noted other than this (and the fact they are the original installer) they have no special rights: for instance if they are not an invoice's creator or on its get or verify permissions list, they cannot get or verify or recover funds of that invoice by calling the canister's API; similarly they cannot arbitrarily transfer funds out of any subaccount with the code base as it is. As for principals on the allowed creators list, they too can call every method of the invoice canister API except adding, removing or getting the allowed creators list.
ㅤThere is a simple implementation of adding a 'delegatedAdministrator' who would also have the ability to add and remove allowed creators, originally as an optional deployment argument and then with an "official" getter and setter in the API list, as this might be a useful feature for the invoice canister to have (a developer could setup and maintain the canister for a storefront, and give the storefront the "administrator" access control while still being able to do DX tech support/dev ops; or for blackholing the canister), but this is left as an exercise for the developer to implement as there are libraries that can do that and much more out there if that is a needed feature (that code is tag commited5).ㅤTo see all the related code:
-
src/module/Types.mo
(lines 65-133, add/remove/get allowed creators list API types). -
src/Invoice.mo::allowedCreatorsList_
(line 66, stable principal array). -
src/Invoice.mo::add_allowed_creator()
(lines 232-252). -
src/Invoice.mo::remove_allowed_creator()
(lines 539-575). -
src/Invoice.mo::get_allowed_creators_list()
(lines 769-805). -
test/e2e/src/tests/allowedCreatorsList.test.js.mo
(entire file, test subsuites for all the above methods). -
test/e2e/src/tests/*.test.js
(used by most other test suites as well).
-
- dfinity/invoice-canister#19
- Extensive Motokodoc and in-method-body comments literally everywhere / generated
dev docs
. - All testing is also extensively commented:
- Standalone (non-generated) developer docs in
docs/
.
- Extensive Motokodoc and in-method-body comments literally everywhere / generated
- dfinity/invoice-canister#26
ㅤThreshold encryption for E2E processing in Motoko not yet available; implications of this documented in Motokodoc and discussed in developer docs indocs/
.
- dfinity/invoice-canister#29
- (1) "redundant argument in verify_invoice:..."
✶ Method completely redone for ICRC1 integration, and this argument is no longer there6. - (2) "In verify_invoice the from_subaccount is re-computed..."
✶ If all the addresses are to be computed once, then there is going to be lot of redundant data as each invoice would need it's own subaccount blob, creator subaccount blob, and creator subaccount address. The latter two could be put into their own hashmap or collection, but all the addresses will still require computation, which in turn requires testing to verify functionality. In other words as long as the methods to do this are clearly defined with a single responsibility (as well as well tested), it represents less of a potential problem particularly if there's extensive documentation/commentary explaining what and how does what and how. If at a later point a developer wants to add this as an extra layer of security (through redundancy) as this is an invoice bearing smart contract, they can implement what's needed with the given implementation which as clearly defined and well tested (theICP
andICRC1
SupportedToken.Adapter
modules). - (3) "verify_invoice still has a TODO that should be removed..."
✶ Method redone for ICRC1 integration and this is no longer there; as well as themotoko-seller-client
's implementation of the invoice canister has also been updated as well. - (4) "in refund_invoice let replaced = invoices.put(i.id,..."
✶ This was never a part of the code base that was a part of the offial branch in the dfinity examples repo afaik; in any case it is no longer there as the refund method was also redone. - (5) "get_invoice returns invoice by default:..."
✶ Also did not make it into the official branch of the dfinity examples code base and is no longer an issue. - (6) "get_invoice permissions: IIUC one always..."
✶ A buyer's principal does need to be in the permissions to either get or verify an invoice; it is not technically required though. In any case it is clearly mentioned in the non-generated developer docs. - (7) "unused code: defaultSubAccount..."
✶ No longer an issue. When a default subaccount is needed, it is done by the theprincipal
library introduced with [SEC-F12] to generate the default subaccount account identifier.
- (1) "redundant argument in verify_invoice:..."
ㅤAlmost every test title literal (or its parent test title literal) will have the involved method and/or expected input/output annotated as suffix to make it easier to parse at a glance.
ㅤIn addition to these tasks, there is unit and E2E testing with as complete coverage as could be reasonably implemented (every public method of the SupportedToken
module (ICP.Adapter
, ICRC1.Adapter
and the set of common methods at that's file module scope; as well as the invoice canister's API methods) so that in general every well defined input and output should have its own test case.
ㅤThe exception to this is all the possible return types from the token-ledger canisters, although all the #Ok
and many of #Err
types are tested by integration within the rest of the E2E test suites. For all the other token-ledger canister's transfer #Err
results, that are not part of the normally expected flow of logic for a given invoice canister API method, they are returned as the argument of that supported token's SupportedToken
variant tag as the invoice canister's returned #SupportedTokenTransferErr: SupportedToken.TransferErr
#err kind
result type. This can be verified in the E2E transfer.test.js
suite in the subsuite describe("Prevent Caller from Transferring More than their Balance...")
for each of the four tokens added, as the invoice canister transfer call result contains the corresponding token-ledger canister's own #Err
result type when this occurs. In other words, when a user calls the invoice canister's transfer
method such that the actual call to the token-canister ledger finds the transfer amount requested is less than the available balance, the invoice canister returns to the caller the #err kind
result:
ㅤ#SupportedTokenTransferErr : #InsufficientFunds : <#ICPx : { balance: { e8s : Nat64 } } | #ICRC1x : { balance : Tokens}>
ㅤThis demonstrates this should work as expected for the other #Err
result types as well. Additionally, each inter-canister call is wrapped in its own try/catch
so that if those token-ledger canister's trap unexpectantly, it should be returned to the original caller of the invoice canister as the #err kind #CaughtException : Text
where the Text
argument is the caught error's Error.message(e)
value.
ㅤThe motivation for this is to give the developer the choice in deciding how to handle what to do in each such case--and this is only for when there is a problem, otherwise the token-ledger canister's returned results are rewrapped by the invoice canister to give the expected API result type.
ㅤFinally on that note the two mock token-ledger canisters created should return most of those #Err
types correctly (as well as the #Ok
ones), with the exception of the ICRC1's #TemporarilyUnavailable
and #GenericError
variant tags, if a developer wants to target specific conditions producing error results. Adding a callback to auto-return either of those or any of the others, or trigger an exception to be caught should not be an unreasonable exercise if such is needed.
ㅤOne final note is that the E2E test suite recover_invoice_subaccount_balance.test.js
involves enough different functionality it demonstrates the majority of the required functionality for completing the work required of this bounty.
Footnotes
-
While a hashmap would have sufficed, these should help at least with more robust testing if needed. ↩
-
The invoice canister now does all its amounts arithmetic in
Nat
before setting any token specific transfer args. ↩ -
In the event zero payment invoices are needed, this would likely be a byproduct of implementing a 'status' field for invoices to handle other features like full refund functionality and is left as an exercise for the developer. ↩
-
Git commit tag 816165a-LibrariesEquivalence 816165a7ecd07760548570dc7e2e32a579f788b2 - "Created new utility module of equivalent ICP related functionality and demonstrated working equivalence in unit tests" -
test/unit/Test.mo
. ↩ -
Git commit tag fecdaa1-delegatedAdministrator fecdaa184ab94416b87a70881fc1db686dbbd98e - "Not very complicated code adding a setting/getter to add a delegated administrator. E2E tests also exist in the next commit." -(lines 133-164 in
src/invoice/main.mo
) ↩ -
Before the major refactoring, it was specifically removed see git commit tag 942f066-redundantargremoved 942f066d1a2be48b585788b417d3e5d8edb0636d "Just a tag showing the arg was specifically removed at one point."
src/invoice/ICPLedger.mo
line 143-147 ↩