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

SRC-21: add out-of-band fee-on-transfer token standard #103

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

moodysalem
Copy link
Contributor

No description provided.

@moodysalem moodysalem changed the title SRC-X: add fee on transfer interface SRC-X: add out-fo-band fee-on-transfer token standard Jul 29, 2024
@moodysalem moodysalem changed the title SRC-X: add out-fo-band fee-on-transfer token standard SRC-X: add out-of-band fee-on-transfer token standard Jul 29, 2024
SNIPS/snip-x.md Outdated Show resolved Hide resolved
SNIPS/snip-x.md Outdated
created: 2024-07-29
---

## Simple Summary
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a flow diagram to explain how the normal scenario unfolds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some context as well as some methods to the interface to the specification

Could use some feedback from FOT developers on how it is proposed to work

Choose a reason for hiding this comment

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

@moodysalem, thanks for proposing this out-of-band FOT standard. As a FOT developer, I have some thoughts:

Implementation seems feasible. Curious about what specifically makes FOT tokens difficult in Ekubo's architecture?

I think pre-paying tax might add friction, but it's probably manageable. Your approach could prevent 'k' invariant issues in pools. It seems easier to implement since the transfer function doesn't alter token amounts in the pool.

SNIPS/snip-x.md Outdated Show resolved Hide resolved
SNIPS/snip-x.md Outdated Show resolved Hide resolved
SNIPS/snip-x.md Outdated
- DApp stores off-chain metadata about whether a token is fee-on-transfer, and implements the following logic if it is
- When depositing this token into a dapp, the `recipient` is the Dapp contract:
- Dapp calls `compute_fees_required(user, dapp_contract, amount)` off-chain
- Dapp adds `pay_fees(computed_fees)` call to the list of calls before all dapp calls
Copy link

Choose a reason for hiding this comment

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

shouldn't this be

"Dapp adds pay_fees_for_sender(user, computed_fees)call" given that the user is the sender? current call defines the Dapp as the sender

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when i say 'dapp adds xyz to list of calls', i mean calls that are made from the user's account. it's usually easier to not include the user's address in the list of calls, so pay_fees would implicitly be for the user's account

Copy link

@TAdev0 TAdev0 Aug 1, 2024

Choose a reason for hiding this comment

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

Oh yes makes sense.. my bad. I understood it as the Dapp making the calls to the token contract that implements the interface with calldata passed by the user (which indeed doesn't make sense as it's the user that pays the fee, not the dapp)

SNIPS/snip-x.md Outdated Show resolved Hide resolved
SNIPS/snip-x.md Outdated Show resolved Hide resolved
SNIPS/snip-x.md Outdated Show resolved Hide resolved
@moodysalem
Copy link
Contributor Author

Could someone assign a SNIP number?

@Eikix Eikix changed the title SRC-X: add out-of-band fee-on-transfer token standard SRC-21: add out-of-band fee-on-transfer token standard Aug 15, 2024
@Eikix
Copy link
Collaborator

Eikix commented Aug 15, 2024

Could someone assign a SNIP number?

Assigning the number 21 to this SNIP. Skipping number 20 to avoid confusion with ERC-20

@leo-starkware
Copy link
Collaborator

Hey @moodysalem, can you add a sections "Rationale" and "Security considerations" (to keep syntax uniform among snips). IMO you can delete section "Implementation" and "History" for the time being.

I think it would help to add more context on how this standard interacts with the ERC-20 token standard, e.g. does this standard require that the token be an ERC-20 token? The inteface should lie in the token contract? Should there be events?

I also think that adding a use case would be helpful. It's not clear to me what problems this standard comes to solve (this isn't a blocker for merging, but adding context may make it a better snip IMO).


The expected usage flow is as follows:

- DApp stores off-chain metadata about whether a token is fee-on-transfer, and implements the following logic if it is
Copy link
Collaborator

Choose a reason for hiding this comment

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

syntax: below you write "DApp", "Dapp", "dapp". Better sticking to one choice.

Copy link

There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale.
This PR will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

@github-actions github-actions bot added the stale label Oct 22, 2024
@leo-starkware
Copy link
Collaborator

@moodysalem ping on the above, to avoid github-bot closing the PR

@github-actions github-actions bot removed the stale label Oct 28, 2024
Copy link

There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale.
This PR will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

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.

5 participants