Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add SIP: Composable Fungible Tokens with Allowance #154
base: main
Are you sure you want to change the base?
Add SIP: Composable Fungible Tokens with Allowance #154
Changes from 5 commits
9ccc7e1
e64d427
41ad33e
4ee00d1
e392b1c
9dbdd0d
ef9c451
c7f7325
3cee6da
f37ae0c
36cbc74
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have post-conditions. Therefore, not very dangerous IMO, but it is useful to allow implementation of send-many contracts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This security model (no transfers for tx-sender) limits composability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is another reference: https://app.sigle.io/friedger.id.stx/HuOT9tNQC8fTXOsK28D7e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no transfer for
tx-sender
limits the 1-hop DeFi that current works de-facto. If I reason right for 2-hops or 3-hops DeFi then thetx-sender
approach does not work (see new Rationale).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tx-sender
does not limit transfers to only 1 hop. You can do as many hops as you want (within the limits of execution costs of course).In fact it provides greater composability and flexibility than
contract-caller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
allow-contract-caller
like in pox contractThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems too long, what about
allow
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
disallow-contract-caller
like in pox contractThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems a bit long, maybe
disallow
??There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better
get-allowance
or evenget-allowance-contract-caller
to be similar toget-balance
using in SIP-10 andget-allowance-contract-caller
used in pox contracts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok sounds good. Do we want the standards to always return a
response
or it can be a pureuint
value?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trait can't have functions that returns anything but
response
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would defined only the newly added functions in the trait. Contracts can implement both the sip-10 trait and this trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I though about doing that, but I felt it was better to replace the whole trait altogether to avoid the hazzle of implementing 2 traits and avoid the confusion with
tx-sender
that is one of the main points of the SIP. I was afraid that people implement both traits but continue checkingtx-sender
and avoding the new functions. From my point of view is easy to do a simpler SIP without the other functions but I was afraid that it will not replace the prev standard.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I think I might limit this Improve Proposal to newly added functions, then the market can decide if they want to use allowances or limit to current approach. We can publish Best Practices for SIP10 that should be a combination of:
tx-sender OR contract-caller
check, post-conditions. If the main web3 apps do not embrace allowances no one will probably do it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is Phishing? Could you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will include more information in the doc.
These days is mainly a Fake Airdrop, read but you probably have alread read it https://www.coinfabrik.com/blog/tx-sender-in-clarity-smart-contracts/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this can't be done with SIP-10. In 5. the user could also transfer the amount of tokens required for
example-defi-service
. Are there blocks between 5. and 6.?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be done with SIP10 with
transfer
only if tokens checktx-sender
, if lets say DeFiA uses DeFiB and DeFiA transfer your tokens from your wallet to DeFiB. Then you give total control to the DeFiA and also to DeFiB called by DeFiA. Not sure your tokens can be retrieved properly because your tokens are jumping from your wallet to DeFiB.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, why do we need allowances? Is it only about the issue of tx with allow mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you have DeFiA is a YieldAggregator and DeFiB is a YieldService, then the following 3 current approaches does not work I think:
a) User calls
defi-service
from DeFiA.b) DeFiA does
transfer(User,DeFiA)
, good tx-sender.c) In the same transaction DeFiA cannot do
transfer(DeFiA,DefiB)
because the tx-sender is still User.a) User calls
defi-service
from DeFiA.b) DeFiA does
transfer(User,DeFiB)
, good tx-sender directly from User.c) Incorrect, DeFiB does not supports receiving random token from third parties without calling
another-defi-service
from DeFiB.a) User calls
defi-service
from DeFiA.b) DeFiA does
transfer(User,DeFiA)
, good tx-sender.c) DeFiA calls
another-defi-service
from DeFiB.d) Incorrect, tx-sender is still User, if DeFiB calls
transfer(DeFiA,DeFiB)
it has no permission.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 1. c) DeFiA can use
as-contract
to switch context, but needs to be careful. Furthermore, the token can allow transfers where sender is contract-caller OR tx-sender. Then no context switch is necessary.In 3. c) Similarily, DeFiA can use
as-contract
to switch context, but needs to be careful.In 3. DeFiA could not do b) (transfer tokens from User to DeFiA), but let DeFiB do it in d).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okey, so the solution for you is to change
tx-sender
to local contract by usingas-contract
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@friedger https://forum.stacks.org/t/escrow-contract/10642
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are SIP-10 token contracts on mainnet like vibe tokens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I see is that without allowances you are giving total control to any DeFi service (that only checks
tx-sender
) to grab any amount of any of your SIP10 Tokens?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, there are 40 contracts with
approve
function, but sometimes they use it for access control or the signature is different, they also include the owner in the signature.https://github.com/search?q=repo%3Aboomcrypto%2Fclarity-deployed-contracts+define-public+%22%28approve+%22&type=code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For that we have post conditions @jo-tm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only 5 tokens seems to have
allowance
orallowance-of
I think the rest usesapprove
for Access Control Roles.https://github.com/search?q=repo%3Aboomcrypto%2Fclarity-deployed-contracts+define-public+%22%28allowance-of+%22&type=code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear whether SIP-10 tokens that use tx-sender are compatible with this SIP. Could you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I wanted to limit the
transfer(from, to, amount)
to(transfer (to, amount))
and the sender is alwayscontract-caller
, removing first parameter I kept thefrom
parameter to maintain compatibility with current approach.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signature is the same but the security model is not, therefore it is not compatible any more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current SIP10 Tokens are only compatibles in that they both will work on Leather Wallet using the same
transfer(user,recipient,amount)
. Also to use a Single DeFi service, no Composability. If you usetransfer
to use a simple DeFi service like current approaches it will work. Not sure what the Stacks Swap Dapps are doing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
improved backwards compatiblity section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That contract is not sip-10 compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have to improve that.