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

wip: support native ether #1262

Merged
merged 66 commits into from
Jan 5, 2021
Merged

wip: support native ether #1262

merged 66 commits into from
Jan 5, 2021

Conversation

hexyls
Copy link
Contributor

@hexyls hexyls commented Oct 30, 2020

Closes #676

Includes native asset support for ethereum (mainnet, rinkeby), sokol, and xdai.

xDAI
Replaces: #1464

PR does not include verification, we can add that later once the kleros/curate-xdai subgraph is up.

  1. Add xDai as a new network to your wallet:
    Name: xDai
    RPC: https://rpc.xdaichain.com/
    Chain ID: 100
    Symbol: xDai
    Block explorer: https://blockscout.com/poa/xdai
  2. Use https://bridge.xdaichain.com/ to convert dai on Ethereum to xDai
  3. Use native xDai to fund/buy
  4. Alternatively, send some xDai to the wrapped xDai address: 0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d

@hexyls hexyls marked this pull request as draft October 30, 2020 01:40
@pimato
Copy link
Contributor

pimato commented Nov 1, 2020

Hey @hexyls can you include Ether into the UI so I can test it?

for the time being, we should include both tokens Ether/Wrapped Ether in the the asset balance dropdown so the users can do whatever they want. So only if the user picks native Ether, we need to go the gnosis safe batches all actions together route.

@pimato pimato added this to the Version 1.2.0 milestone Nov 1, 2020
@hexyls
Copy link
Contributor Author

hexyls commented Nov 2, 2020

Funding with native ether should work at this point but does not.

Some notes:

  • contract-proxy-kit converts tx values to numbers which ethers.js does not like.
  • In my tests I confirmed that once a proxy's implementation is upgraded to Gnosis Safe v1.2.0 you can successfully send ether to the proxy and wrap it in a single transaction.
  • However as soon as the multisend contract is involved contract-proxy-kit will estimate that the transaction will revert.

Will continue to explore...

@hexyls
Copy link
Contributor Author

hexyls commented Nov 2, 2020

Yep there is a problem with the MultiSend contract. Again it is the same issue that both Gnosis Safe v1.1.1 and the current Proxy factory have.

I deployed a new MultiSend contract on Rinkeby and was able to get multisend with ether wrapping working.

However the conversion of tx values to numbers in contract-proxy-kit is still a problem.

@hexyls
Copy link
Contributor Author

hexyls commented Nov 4, 2020

Today I monkey patched the cpk library to fix the transaction value encoding issue.

Once this proposal passes to add / index WETH markets on Rinkeby we should be ready to test market creation in native ether. I have actually made a successful transaction that should have created a market but it was not indexed.

Copy link
Contributor

@kadenzipfel kadenzipfel left a comment

Choose a reason for hiding this comment

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

Great work!

@@ -67,6 +70,8 @@ interface KnownTokenData {
order: number
}

export const pseudoNativeAssetAddress = '0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE'
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiousity, is there any particular reason you capitalized the address like this?

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 copied this from the 0x Project I think 🤔 and this is the checksum of that address, for us we could use any value here though cause we don't use the address in any transactions.

@hexyls hexyls mentioned this pull request Dec 22, 2020
Copy link
Contributor

@kadenzipfel kadenzipfel left a comment

Choose a reason for hiding this comment

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

LGTM!

@pimato
Copy link
Contributor

pimato commented Jan 5, 2021

I found issues with the asset dropdown ( #1499 ) , but should be fixed in a separat PR! Looks good to me!

Copy link
Contributor

@Mi-Lan Mi-Lan left a comment

Choose a reason for hiding this comment

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

Looks good ! Tested xdai markets they work good, native eth works great also!

  • I would suggest for the xDai instead of verify section having this error we hide the whole section for xDai this could be created as seprate issue so we publish this feature
    Screenshot 2021-01-05 at 22 09 22
  • Also i found that multiple currencies on xDai don't work, below is how it looks and I added xDai to it...
    Screenshot 2021-01-05 at 22 14 20

Copy link
Contributor

@Mi-Lan Mi-Lan left a comment

Choose a reason for hiding this comment

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

LGTM!

@pimato pimato merged commit 359673d into master Jan 5, 2021
@pimato pimato deleted the feature/support-native-ether branch January 5, 2021 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include ETH as funding asset by using the gnosis proxy kit
5 participants