Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Payroll contract example #94

Draft
wants to merge 11 commits into
base: develop
Choose a base branch
from
Draft

Payroll contract example #94

wants to merge 11 commits into from

Conversation

twitu
Copy link
Collaborator

@twitu twitu commented Nov 6, 2022

The contract demonstrates a simplified but relatable real world scenario where a payroll management contract can add and pay employees.

Among other things it demonstrates how and where to use macros for Rbac and patterns around collections, storage keys and Promises and handling errors in a non-trivial example.

Next step is to handle more error cases, include a multi-sig approval in this and create test cases to show how it works.

Copy link
Contributor

@encody encody left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding test cases for this contract. Alternatively, put it in the examples folder if it is not intended to be part of the workspaces test suite.

workspaces-tests/src/bin/payroll_example.rs Outdated Show resolved Hide resolved
workspaces-tests/src/bin/payroll_example.rs Outdated Show resolved Hide resolved
workspaces-tests/src/bin/payroll_example.rs Outdated Show resolved Hide resolved
workspaces-tests/src/bin/payroll_example.rs Outdated Show resolved Hide resolved
@twitu
Copy link
Collaborator Author

twitu commented Nov 11, 2022

One limitation with the multisig implementation is that it does not allow different roles to approve the action. So in this particular example if I want the manager and the employee to approve the action of disbursing pay it's not possible because they have different roles. If the multisig were to allow multiple enum variants to approve the same action, it's still possible that two employees could approve the action, or a different employee could approve.

Here we want to specific accounts to make a combined approval.

I don't think the default implementation supports such a pattern. This can be documented more clearly that the approval requires m votes among n equal peers (equal as in the same role).

For this example will a custom implementation be more useful or is there some other helpful macro?

@encody
Copy link
Contributor

encody commented Nov 15, 2022

So the #[simple_multisig] macro is the K out of N approval scheme. Maybe it should be renamed? The approval system is generic enough to allow approvals from entities of different standing, however.

@twitu
Copy link
Collaborator Author

twitu commented Nov 20, 2022

I believe simple_multisig is a common terminology in payments and contracts. It's just that I missed it when reading the documentation. Still the documentation can be updated to reflect in simpler terms that, as you say, it's a K out of N voting scheme and currently the voting can only be done among users of the same role.

@twitu
Copy link
Collaborator Author

twitu commented Nov 20, 2022

In the meantime I'll look into implementing a custom approval flow for this. It's for the best I think since the example will demonstrate a slightly more complex approval implementation.

Add documentation/design comments in TODO notes
@twitu
Copy link
Collaborator Author

twitu commented Nov 30, 2022

I've updated the example with a custom approval management. I've also noted down my documentation and design comments as TODO in src/approval/mod.rs. Overall I feel that the approval manager is well designed and guides the programmer to good design. But it is also very generic and needs more documentation and examples to help understand how to actually use it.

Any suggestions for improving the example?

There is one thing left to make it complete i.e. to subtract the logged hours once the transfer is complete.

@twitu
Copy link
Collaborator Author

twitu commented Dec 4, 2022

I'm working on the sandbox tests and I often get errors like this.

thread 'successful_request' panicked at 'called `Result::unwrap()` on an `Err` value: ExecutionResult { total_gas_burnt: 5079091640520, transaction: ExecutionOutcome { block_hash: `9gcZ6Xr2EP49T5BnT17MhKGZKkopGroisfnTsEPhg18u`, logs: [], receipt_ids: [`9qDj5a2auqXNhaUVeajyvxmUUt4tP4R8J4peCYUZZTU1`], gas_burnt: 2427954539010, tokens_burnt: 242795453901000000000, executor_id: AccountId("dev-20221204143310-60254843734169"), status: SuccessReceiptId(9qDj5a2auqXNhaUVeajyvxmUUt4tP4R8J4peCYUZZTU1) }, receipts: [ExecutionOutcome { block_hash: `BNTrFnQ9EFXDf2QRuf6FkVDBbG2aVa3vMJ64tD2SAEw1`, logs: [], receipt_ids: [`4BCGPLwqny35MnjDJqqfFvE9SVJkAANoe92bXNB4AXSd`], gas_burnt: 2427954539010, tokens_burnt: 242795453901000000000, executor_id: AccountId("dev-20221204143315-66270547170103"), status: Failure(ActionError(ActionError { index: Some(0), kind: FunctionCallError(MethodResolveError(MethodNotFound)) })) }, ExecutionOutcome { block_hash: `7QV8P9UnJBixPCFSotBcxB4uawXhbMDbiWqzWgBfZyik`, logs: [], receipt_ids: [], gas_burnt: 223182562500, tokens_burnt: 0, executor_id: AccountId("dev-20221204143310-60254843734169"), status: SuccessValue(``) }], value: ActionError(ActionError { index: Some(0), kind: FunctionCallError(MethodResolveError(MethodNotFound)) }) }', /Users/twitu/.cargo/registry/src/github.com-1ecc6299db9ec823/workspaces-0.6.1/src/result.rs:222:28

It doesn't actually mention which method call which makes it much harder to debug. Is there anyway to expose this information? It'll certainly help newcomers using repo.

In the stack trace it points to the last line of the test. Although I think it fails earlier because removing the last line makes it point to the next last line.

    93: dbg!(&result);
    94: dbg!(&result.borsh::<u8>());
  20: payroll::successful_request
             at ./tests/payroll.rs:94:5
  21: payroll::successful_request::{{closure}}
             at ./tests/payroll.rs:77:7
  22: core::ops::function::FnOnce::call_once

@twitu
Copy link
Collaborator Author

twitu commented Dec 4, 2022

Also I found that there are no examples where the output of a transaction call is used. While trying to do this I found that the result of a transaction is actually the base64 encoding of the return value.

However the ExecutionResult docs type does not mention this and does not provide any method to decode the base64 value. Is this handled using a helper? or some utility script?

@twitu
Copy link
Collaborator Author

twitu commented Dec 17, 2022

A custom implementation of ApprovalManager is pretty non-trivial to use. After initializing the configuration in the contract #[init] function, it still gives not initialized errors when running tests.

impl Payroll {
    #[init]
    pub fn new(owner: &AccountId) -> Self {
        let mut contract = Payroll {
            hourly_fee: 1000,
            logged_time: UnorderedMap::new(PayrollKey::LOG),
        };
        <Payroll as ApprovalManager<PayrollAction, PayrollApproval, PayApprovalConfiguration>>::init(PayApprovalConfiguration {});
        contract.add_role(owner, &Role::Manager);
        contract
    }
}
thread 'test_disburse_payment' panicked at 'called `Result::unwrap()` on an `Err`
<--snip-->
value: ActionError(ActionError { index: Some(0), kind: FunctionCallError(ExecutionError("Smart contract panicked: init must be called before use")) }) }'

workspaces-tests/Cargo.toml Outdated Show resolved Hide resolved
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.

2 participants