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

Adding support for MiniMe tokens #8

Merged
merged 16 commits into from
Aug 23, 2021
Merged

Adding support for MiniMe tokens #8

merged 16 commits into from
Aug 23, 2021

Conversation

brickpop
Copy link
Contributor

@brickpop brickpop commented Jul 30, 2021

The following PR allows to generate proofs for MiniMe Tokens.

It still does not:

  • Support proofs for checkpoints that are defined in parent contracts

It also:

  • Refactors the common logic on common.ts
  • Splits ERC20 and MiniMe logic away
  • Renames many types for better clarity and consistence (some are breaking)
  • Abstracts away tasks like computing the balance slot
  • Performs lighter map slot discoveries
  • Adds more and better tests

Reference:

@brickpop brickpop requested a review from ed255 July 30, 2021 17:30
Copy link

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

I've added some small requests / notes.

package.json Outdated Show resolved Hide resolved
src/common.ts Show resolved Hide resolved
src/common.ts Outdated Show resolved Hide resolved
@ed255
Copy link

ed255 commented Aug 2, 2021

I have one question regarding the functionalities of this library. In this PR you have implemented the methods to get and verify a proof of holding some Minime tokens, which is the functionality provided in this module https://github.com/vocdoni/storage-proofs-eth-go/tree/master/token/minime. I don't see the methods to get and verify a proof of holding a Mapbased token (like this https://github.com/vocdoni/storage-proofs-eth-go/tree/master/token/mapbased). Is the Mapbased funcionality not needed? Or is the verification of a mabpased token holder proof implemented somewhere else? In particular I'm looking for a function like this one (verify that the holder had the targetBalance in the token contract when it had the storageRoot):

func VerifyProof(holder common.Address, storageRoot common.Hash,
	proof ethstorageproof.StorageResult, mapIndexSlot int, targetBalance, targetBlock *big.Int) error {

@ed255
Copy link

ed255 commented Aug 2, 2021

I have a second question about the integration between this library and the smart contracts.
Currently we have a storage proof verification function in solidity at https://github.com/vocdoni/dvote-solidity/blob/40340c6efcb1d76967ac8adcfb0387c851814806/contracts/lib.sol#L371 which is used by some contracts.
And in this repository there are functions to get the storage proofs. Where is the code that transforms the storage proof types returned by storage-proofs-eth-js into the format that the solidity function expects? Is the storageProofsRLP returned by EthProvider.fetchProof compatible with the verify solidity function? Is there any tests for this integration?

@brickpop
Copy link
Contributor Author

brickpop commented Aug 2, 2021

I have one question regarding the functionalities of this library. In this PR you have implemented the methods to get and verify a proof of holding some Minime tokens, which is the functionality provided in this module https://github.com/vocdoni/storage-proofs-eth-go/tree/master/token/minime. I don't see the methods to get and verify a proof of holding a Mapbased token (like this https://github.com/vocdoni/storage-proofs-eth-go/tree/master/token/mapbased). Is the Mapbased funcionality not needed? Or is the verification of a mabpased token holder proof implemented somewhere else?

The point is that this PR is changing a repo originally meant to deal with ERC20's only, into a repo with shared primitives that are reused by different use cases (ERC20, MiniMe, etc).

In the case of ERC20, the functionality is implemented here but I agree that even this could be almost all moved to the common module. However, since it explicitly depends on the ERC20 ABI, I left it here.

Maybe the second half of the body can be factored out

@brickpop
Copy link
Contributor Author

brickpop commented Aug 2, 2021

In particular I'm looking for a function like this one (verify that the holder had the targetBalance in the token contract when it had the storageRoot):

func VerifyProof(holder common.Address, storageRoot common.Hash,
	proof ethstorageproof.StorageResult, mapIndexSlot int, targetBalance, targetBlock *big.Int) error {

Agree. To me this feature makes more sense for gateways that it does for clients. It still follows the initial approach from Jorge, essentially verifying that the proof is correct, by itself. But as I was coding, I saw the difference between verifying the proof against the balance+token or verifying its integrity and assume that the balance matches (callers used to do this, by now).

I was thinnking of another PR to address this, but we can include this here... and make some more breaking changes now

@brickpop
Copy link
Contributor Author

brickpop commented Aug 2, 2021

I have a second question about the integration between this library and the smart contracts.
Currently we have a storage proof verification function in solidity at https://github.com/vocdoni/dvote-solidity/blob/40340c6efcb1d76967ac8adcfb0387c851814806/contracts/lib.sol#L371 which is used by some contracts.
And in this repository there are functions to get the storage proofs. Where is the code that transforms the storage proof types returned by storage-proofs-eth-js into the format that the solidity function expects? Is the storageProofsRLP returned by EthProvider.fetchProof compatible with the verify solidity function? Is there any tests for this integration?

The primitives are implemented here and tested here.

Also, there is the simple way of fetching proofs but also, getting the full block header and data, so that we can match it with the block hash: https://github.com/vocdoni/storage-proofs-eth-js/blob/f/mini-me/src/common.ts#L33-L50
This is where the functions above are used.

@ed255
Copy link

ed255 commented Aug 2, 2021

In particular I'm looking for a function like this one (verify that the holder had the targetBalance in the token contract when it had the storageRoot):

func VerifyProof(holder common.Address, storageRoot common.Hash,
	proof ethstorageproof.StorageResult, mapIndexSlot int, targetBalance, targetBlock *big.Int) error {

Agree. To me this feature makes more sense for gateways that it does for clients. It still follows the initial approach from Jorge, essentially verifying that the proof is correct, by itself. But as I was coding, I saw the difference between verifying the proof against the balance+token or verifying its integrity and assume that the balance matches (callers used to do this, by now).

I was thinnking of another PR to address this, but we can include this here... and make some more breaking changes now

Thanks for the clarification. If you decide to address this in another PR that's fine by me! If you want to extend this PR it's also fine by me :) I just wanted to make sure you were aware of this.

@brickpop brickpop merged commit d3a067c into main Aug 23, 2021
@brickpop brickpop deleted the f/mini-me branch August 23, 2021 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants