status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.88k stars 983 forks source link

Revert to TtT contract with on-chain tribute storage #8294

Closed rachelhamlin closed 3 years ago

rachelhamlin commented 5 years ago

Description

Type: Feature

Summary: As a result of the issues found in #8129, we need to move storage of a user's tribute setting from IPFS to the TtT contract.

This means that we are also removing the personalized message from v1 of the feature.

Expected behavior

TtT works in 100% of scenarios.

Settings flow no longer contains option to set a personalised message. Chat flow only displays system message requesting tribute amount, no personalized message.

image

Actual behavior

TtT is failing to send and receive information from IPFS.

yenda commented 5 years ago

@3esmit is the simple contract already deployed? could you provide the address on testnet?

3esmit commented 5 years ago

@yenda Deployed in Ropsten. Let me know if you need a different network

Version without messages and default fee of 0.1 value: https://ropsten.etherscan.io/address/0x45d8274c1a4c6b5b956e8d0f44a9e78209b0bb77#code browser/MessageTribute.sol : bzz-raw://b3fc092f9b99fcc89693c9adb553acd2c01b19e7d585b2c58d600be6fe43c99f metadata.json : bzz-raw://cbae2ca4219b4547c6fc3e8e7a0f3a12081b2a0cca376c72b2b40dc15d5b4dd0

3esmit commented 5 years ago

Ropsten https://ropsten.etherscan.io/address/0x45d8274c1a4c6b5b956e8d0f44a9e78209b0bb77#code Rinkeby https://rinkeby.etherscan.io/address/0x63bb739fccdd08cf32defacfabad420d657259e7#code Goerli https://blockscout.com/eth/goerli/address/0x63bb739fccdd08cf32defacfabad420d657259e7/contracts

yenda commented 5 years ago

@3esmit I don't think there should be a default tribute amount can you remove it? @rachelhamlin

rachelhamlin commented 5 years ago

Yes please! 0 default.

yenda commented 5 years ago

also I don't think we should have a way to set a default because it makes the reset method useless, the only place I see it useful is to remove tribute, but if instead it goes back to the default value it is misleading and probably not something someone will want. For this iteration we can make things as simple as possible. Btw idealy the method names should be getTribute(address), setTribute(uint256), removeTribute()

3esmit commented 5 years ago

I did the changes as you requested. Kind reminder that I believe a default small fee is an important step against spam.

I didnt included a removeTribute() because you can use setTribute(0). The contract with default fee needed a resetFee function so it would start using the default again. As there is no default, the remove/reset tribute is not required.

This can be a boomer when we have separated Wallet and Identity, so I started thinking on how one wallet can pay for a chat only identity tribute to talk. So I did two versions, one with MessageSigned capability, meaning that is possible of an externally owned account which don't hold any funds to sign a message and get it deployed by any other address. This is possible using an ethereum signed message support in the contract, however I added a TTL to the message, to prevent it being reused in future, and also prevent user from creating a too large TTL.

Im also in the research how to make an "gas anonymizer", so users dont tie wallet to chat identity, this seems to be possible by using a mixer to pay to the gas relayer. This research is happening in other swarm (Gas Abstraction/Identity/ERC725).

The first version, more simple, is available here: https://ropsten.etherscan.io/address/0xc61aa0287247a0398589a66fcd6146ec0f295432#contracts https://rinkeby.etherscan.io/address/0x326e29956334a352e8d6f24e22d781020311eba6#contracts https://goerli.etherscan.io/address/0x326e29956334a352e8d6f24e22d781020311eba6#contracts Sources: https://github.com/status-im/contracts/blob/aa2d12c253b99b2d3d335db7d37446b2458f1293/contracts/communication/MessageTribute.sol MessageTribute.sol: bzz-raw://af585a5421934776033d257b2f511412d6ac28e1c3fe259d91b88aa38164e826 metadata.json: bzz-raw://4ed0b73f2fe48042ae34f201e6b74a48c533aaf5d424b0101cd29c3f525fcf40

The second version, with message signature support, is available here: https://ropsten.etherscan.io/address/0x048887496c59c55ca703d167becb9112ec32c18f#contracts https://rinkeby.etherscan.io/address/0x084dc950f387675044a1404842805678da3fc5a8#contracts https://goerli.etherscan.io/address/0x084dc950f387675044a1404842805678da3fc5a8#contracts Sources: https://github.com/status-im/contracts/blob/9b916795a89416d63318799cf7bb6711f643bd33/contracts/communication/MessageTribute.sol MessageTribute.sol: bzz-raw://5a8f467bcdc22628f363302b783d6a8c03252ca8d9161e5fb777699f90b224f9 metadata.json: bzz-raw://1870ccff990c1ff63a603d2bcf0f4c6181f2d7350f4831ce2d7a8faa49beb155

The third version, with gas anonymizer support, would feature a similar change, an overload of setTribute function which contains the incentive to a random gas relayer to commit the transaction inchain and get paid in an zk-snarks mixer.

rachelhamlin commented 5 years ago

Thank you @3esmit!

yenda commented 5 years ago

@3esmit Is there a way to use the simple contract and later on add the possibility to use the MessageSigned capability in another one while still using this one as registry so that old versions of status still have access? Also if I set tribute to talk using the MessageSigned capability to which wallet are the tributes going to be paid? If I understand correctly I am still sending the fee to the signer which supposedly doesn't have an account at this address?

yenda commented 5 years ago

@3esmit thanks for the detailed post btw!

Another question, do we really need to emit a log for the simple contract?

3esmit commented 5 years ago

The upgradability is not implemented on this contract. Migration process can be defined in UI, but would be painful. New contract can read from old contract to skip migration of already defined values in older contract.

I can deploy this contract and use a UpgradableInstance.sol (Proxy Contract, uses delegatecall) pointing to it, so we can have upgradability.

3esmit commented 5 years ago

The use of events is not necessary, but they might be important for updating the UI accordingly to the changes.

As a common practice, meaningful state changes on the contract should fire events. Events can improve the performance of the dapp, in an example, the dapp could load values of a list of address in background and cache them in local database, and subscribe to the according events that would update this cache database. When user interacts with this addresses the values would be locally available and would not have to be downloaded from lightserving nodes, improving UX. Also, events can be filtered when they have "indexed" parameters, this allow Dapp to be notified by the blockchain node.
For example in ERC20 event Transfer(address indexed _from, address indexed _to, uint256 _value) The wallet could subscribe to Transfer when from is the address of user, or when to is address of user, can accordingly display an notification in smartphone of this moving of funds, without having to check every transaction in blockchain. Its important to be bolded that events are ephemeral, they cannot be read from the contract, only from transactions, and they are fired automatically by the node when the new block have its transactions validated, so very old events would not be fired with a fast sync, only with slow sync, and this is just one of the caveats of events. See this post about EVM events and problems https://hackernoon.com/ethereum-dapp-infrastructure-2b4f1e6bfa38

However for this version of Tribute-to-Talk, indeed events are not necessary, as the values will be read from the contract always on demand, because we never know what is the new contact user will try to message, so we cannot cache that data locally, and once it's paid, there is no need of keeping sync on it.

For recurring payments/subscription model this events might be interesting depending on the features of the contract, for example, if the subscription value was changed, this could be grabbed from an event, instead of every time reading all subscriptions to see if they changed value.

rachelhamlin commented 5 years ago

The upgradability is not implemented on this contract. Migration process can be defined in UI, but would be painful. New contract can read from old contract to skip migration of already defined values in older contract.

Does this have implications for #8047?