Implement subkeys as a cw1 contract #24

ethanfrey · 2020-07-24T11:52:41Z

This idea was thrown around Summer 2019.

Basically an account that can authorize arbitrary external accounts to spend some limited amount of tokens from it. And in theory perform other actions (like staking).

This implements subkey controls over the native Bank::SendMsg action as part of a cw1-proxy contract.

CLAassistant · 2020-07-24T11:52:46Z

All committers have signed the CLA.

ethanfrey · 2020-07-28T18:58:49Z

@maurolacy I sketched out my ideas on some sort of reuse of code from cw1_whitelist to add the idea of subkeys (which is allowances for native tokens, via BankMsg::Send). Hopefully the code comments and the README make sense.

Can you make a PR off of this to complete the work? I left some panics in the contract code that needs implementing, and comments for the test cases that would be good to have. It should be rather straight-forward (if a non-trivial amount of code).

Once you make one PR implementing it, if you have better ideas on how to promote code reuse (a cleaner way that I did), please make a second PR building on it that makes those changes - feel free to reorganize code in other contracts and packages when doing this)

maurolacy · 2020-07-29T05:57:53Z

@maurolacy I sketched out my ideas on some sort of reuse of code from cw1_whitelist to add the idea of subkeys (which is allowances for native tokens, via BankMsg::Send). Hopefully the code comments and the README make sense.

Can you make a PR off of this to complete the work? I left some panics in the contract code that needs implementing, and comments for the test cases that would be good to have. It should be rather straight-forward (if a non-trivial amount of code).

Once you make one PR implementing it, if you have better ideas on how to promote code reuse (a cleaner way that I did), please make a second PR building on it that makes those changes - feel free to reorganize code in other contracts and packages when doing this)

OK, I'll take a look and ask you if I have any doubts.

Also, implement SubAssign trait for Balance

ethanfrey

Nice start. Left some comments on it

I think the last two commits should be reverted (robust sub and unit test fix) but the allowance implementations look good

contracts/cw1-subkeys/src/balance.rs

contracts/cw1-subkeys/src/contract.rs

contracts/cw1-subkeys/src/state.rs

This reverts commit 67d74d9.

This reverts commit 9bede00.

ethanfrey · 2020-08-02T11:56:20Z

Please ping me when you implement execute and I will give a final review. The allowances look fine

maurolacy · 2020-08-02T18:30:33Z

OK, I've done a first implementation of execute. I have a number of doubts, some of which are reflected in the code.

Please take a look. Once this is right, I'll write the unit tests.

ethanfrey

Looks good. Just missing tests, but besides the minor cleanup I mentioned, it looks correct.

contracts/cw1-subkeys/src/contract.rs

ethanfrey · 2020-08-03T13:01:11Z

contracts/cw1-subkeys/src/contract.rs

@@ -145,7 +172,7 @@ where
        if let Some(exp) = expires {
            allowance.expires = exp;
        }
-        allowance.balance = allowance.balance.sub(amount.clone())?; // Fails if no tokens
+        allowance.balance = allowance.balance.sub_saturating(amount.clone())?; // Fails if no tokens


The comment "fails if no tokens". Is that out of date now?

Correction: that still fails if no tokens. It silently ignores underflows, but doesn't ignore a missing balance entry for that denomination. I think this is an important distinction: an admin could make a spelling mistake when trying to remove an entry, and if this fails silently, he will never be notified of the error.

Nevertheless, if you think this is not desirable / important, I'll change it to always failing silently.

Related: Shouldn't denom be case insensitive, for robustness / simplicity?

Very good distinction to make (underflow vs. no denom) and I agree with it. Nice thinking.

The comment could be a little more verbose and capture that.

As to case insensitive... it is case sensitive in the sdk. we could normalize both stored values and the ones we match to always be lower case before comparing. I guess that would be nice. But I'm not sure if there are issues there. Can you make an issue to add that, and we can add that later, once this is merged. And then we can maybe get some comments on the idea. I am not completely sure about that one.

Me neither... I'll create the issue.

maurolacy · 2020-08-03T13:51:05Z

Looks good. Just missing tests, but besides the minor cleanup I mentioned, it looks correct.

I'm glad I figured it out. I must say in the name of truth that it was mostly based on logic and deductions, as I still lack detailed knowledge of the platform and the SDK.

ethanfrey · 2020-08-03T16:56:57Z

I'm glad I figured it out. I must say in the name of truth that it was mostly based on logic and deductions, as I still lack detailed knowledge of the platform and the SDK.

Please add an issue to https://github.com/CosmWasm/docs2 for all info you would like to have readily accessible when first wirting a contract. We are planning/starting an overhaul of the docs site and input from people just starting with CosmWasm is invaluable.

You an make a new issue or add some thoughts here: CosmWasm/docs-deprecated#5

maurolacy · 2020-08-03T20:56:23Z

Please add an issue to https://github.com/CosmWasm/docs2 for all info you would like to have readily accessible when first wirting a contract. We are planning/starting an overhaul of the docs site and input from people just starting with CosmWasm is invaluable.

You an make a new issue or add some thoughts here: CosmWasm/docs2#5

For me, the most "difficult" part was in some cases to figure out what NOT to do (i. e. what was not necessary). To be fair, this is probably related to my way of approaching things, as I'm no fan of reading lot of docs when starting with something new, and prefer to gain a big picture gradually. Starting from the details, so to speak.

Regarding the "complexities": By example, the fact that for relaying the messages it was enough to set the messages member in the HandleResponse struct, was far from obvious to me. I got it by analysing the already existing functionality in execute for the admin case; and by deducting the behaviour of the SDK.

I must say that the examples are very good, and were very useful in general. cw1-whitelist and cw20-base in particular (of which cw1-subkeys is just an elaboration, or iteration). The API itself is very well thought of, and makes a lot of sense; which is very helpful.

I'll probably elaborate on this in the issue later, but I would say that some short documents, even diagrams, with the aim to gain a big picture quickly, particularly of the interactions and responsibilities of the different parts and frameworks, would be very useful for would-be contract writers.

Finally: these are not simple matters. I mean, the API and the SDK can make writing functional / useful contracts a relatively easy task, but the functionality itself must be very well thought of, as there are many corner cases, this is a delicate area in terms of security, money and finances are involved, etc., etc.

ethanfrey

Very well done @maurolacy I like the tests.

I will merge this in now. Thank you for completing this contract. I look forward to deploying it on our next testnets.

contracts/cw1-subkeys/src/contract.rs

ethanfrey · 2020-08-07T18:56:08Z

contracts/cw1-subkeys/src/contract.rs

+            }
+        );
+
+        // Add to spender4 (new account) (expires = Some)


Nicely covering all the use cases.

It actually seems the zip above did exactly what you wanted it to do.
but setup_test_case needs better rustdoc, so this is expected behavior

I think it's related to the comment above. I'll clarify the code with better naming / comments.

ethanfrey · 2020-08-07T18:58:38Z

contracts/cw1-subkeys/src/contract.rs

+            allowance,
+            Allowance {
+                balance: Balance(vec![
+                    coin(amount1 / 2 + (amount1 & 1), denom1),


I would usually use + (amount1 % 2) rather than + (amount 1 & 1) but they produce the same result.
I guess you are more used to embedded systems than I am.

contracts/cw1-subkeys/src/contract.rs

ethanfrey · 2020-08-07T19:01:39Z

Oops, I realized I had some change requests above, after I merged.
Can you make a small PR with any minor adjustments (mainly the rustdoc for setup_test_case), but anything else you feel like changing as well.

maurolacy · 2020-08-07T19:11:19Z

Sure, I was planning to finish those two remaining tests tomorrow morning anyway.

Sorry to commit / push partial stuff. I'm just used to working that way. I can change it if it's a burden.

ethanfrey · 2020-08-07T20:06:10Z

No, it is great. Please keep pushing.

Two issues.. it is late and I was trigger happy on a merge. And two, I opened this PR, otherwise I would have waited for you to take it out of draft mode before merging.

Generally, open a draft PR when starting something and mark "ready for review" when you feel it is good to merge if approved.

ethanfrey mentioned this pull request Jul 24, 2020

Add CLA bot #1

Closed

ethanfrey added 8 commits July 28, 2020 19:54

Copy cw1-whitelist to cw1-subkeys and add README

ce37ed8

Add subkeys to CI

1e84fc3

Start updating to 0.10

561cda4

Revive Wallet from orphaned branch

81cc586

Layout cw1-subkeys for maximum reuse of cw1-whitelist

c17ec86

More wiring, contract logic plumbed out

34f23c9

Update schema

e89d9e6

Clean up testcode, leave stubs for subkeys

d09db01

ethanfrey force-pushed the subkeys branch from f23a2d9 to d09db01 Compare July 28, 2020 18:55

maurolacy added 5 commits August 1, 2020 08:56

Update cw1-subkeys to cosmwasm 0.10.0

e81503b

handle_increase_allowance implementation

d3f6ccc

decrease_allowance implementation

fb24ec5

More robust sub()

67d74d9

Also, implement SubAssign trait for Balance

Fix: update underflow unit test

9bede00

ethanfrey commented Aug 1, 2020

View reviewed changes

maurolacy added 3 commits August 2, 2020 00:24

Revert "More robust sub()"

35ec17b

This reverts commit 67d74d9.

Revert "Fix: update underflow unit test"

8e895ab

This reverts commit 9bede00.

Option unwrapping using ok_or_else()

5747390

maurolacy added 2 commits August 2, 2020 13:57

sub_saturating implementation

eff292c

handle_execute initial impolementation

014093b

ethanfrey commented Aug 3, 2020

View reviewed changes

Change error type to not_found, remove comments

757afb7

maurolacy added 4 commits August 5, 2020 10:25

Improve underflow vs unexistent denom comment

4335964

Test increase_allowance

71c5d87

cargo fmt

3b9f52a

Test decrease_allowance

41e61dc

ethanfrey marked this pull request as ready for review August 7, 2020 18:59

ethanfrey commented Aug 7, 2020

View reviewed changes

ethanfrey merged commit 3932fd9 into master Aug 7, 2020

ethanfrey deleted the subkeys branch August 7, 2020 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement subkeys as a cw1 contract #24

Implement subkeys as a cw1 contract #24

ethanfrey commented Jul 24, 2020

CLAassistant commented Jul 24, 2020 •

edited

Loading

ethanfrey commented Jul 28, 2020

maurolacy commented Jul 29, 2020

ethanfrey left a comment •

edited

Loading

ethanfrey commented Aug 2, 2020

maurolacy commented Aug 2, 2020

ethanfrey left a comment

ethanfrey Aug 3, 2020

maurolacy Aug 3, 2020 •

edited

Loading

maurolacy Aug 3, 2020 •

edited

Loading

ethanfrey Aug 3, 2020

ethanfrey Aug 3, 2020

maurolacy Aug 3, 2020

maurolacy commented Aug 3, 2020

ethanfrey commented Aug 3, 2020

maurolacy commented Aug 3, 2020 •

edited

Loading

ethanfrey left a comment

ethanfrey Aug 7, 2020

maurolacy Aug 7, 2020

ethanfrey Aug 7, 2020

ethanfrey commented Aug 7, 2020

maurolacy commented Aug 7, 2020 •

edited

Loading

ethanfrey commented Aug 7, 2020

Implement subkeys as a cw1 contract #24

Implement subkeys as a cw1 contract #24

Conversation

ethanfrey commented Jul 24, 2020

CLAassistant commented Jul 24, 2020 • edited Loading

ethanfrey commented Jul 28, 2020

maurolacy commented Jul 29, 2020

ethanfrey left a comment • edited Loading

Choose a reason for hiding this comment

ethanfrey commented Aug 2, 2020

maurolacy commented Aug 2, 2020

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey Aug 3, 2020

Choose a reason for hiding this comment

maurolacy Aug 3, 2020 • edited Loading

Choose a reason for hiding this comment

maurolacy Aug 3, 2020 • edited Loading

Choose a reason for hiding this comment

ethanfrey Aug 3, 2020

Choose a reason for hiding this comment

ethanfrey Aug 3, 2020

Choose a reason for hiding this comment

maurolacy Aug 3, 2020

Choose a reason for hiding this comment

maurolacy commented Aug 3, 2020

ethanfrey commented Aug 3, 2020

maurolacy commented Aug 3, 2020 • edited Loading

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey Aug 7, 2020

Choose a reason for hiding this comment

maurolacy Aug 7, 2020

Choose a reason for hiding this comment

ethanfrey Aug 7, 2020

Choose a reason for hiding this comment

ethanfrey commented Aug 7, 2020

maurolacy commented Aug 7, 2020 • edited Loading

ethanfrey commented Aug 7, 2020

CLAassistant commented Jul 24, 2020 •

edited

Loading

ethanfrey left a comment •

edited

Loading

maurolacy Aug 3, 2020 •

edited

Loading

maurolacy Aug 3, 2020 •

edited

Loading

maurolacy commented Aug 3, 2020 •

edited

Loading

maurolacy commented Aug 7, 2020 •

edited

Loading