Skip to content
This repository has been archived by the owner on Aug 9, 2021. It is now read-only.

feat: move out auth/provider to parent window provider #33

Merged
merged 11 commits into from
May 30, 2020

Conversation

zachferland
Copy link
Contributor

@zachferland zachferland commented May 14, 2020

TODO
[] some added logic with state moved here now so should now add tests for that
[] since full migration available, probably will add here as well and feature flag (mainly just auth and migration of all spaces, full migration will have more account management and linking not added now)
[] small changes to 3boxjs and idw will be pushed up
[] test with more providers (mostly the ui part, can overlay still right now, just need right zindex, later wont want to make that assumption) (maybe just add web3modal to 3boxjs example)

@zachferland zachferland force-pushed the feat/external-provider branch 6 times, most recently from 3908b4d to 2114ee0 Compare May 21, 2020 00:09
@zachferland zachferland force-pushed the feat/external-provider branch from 2114ee0 to 03d1169 Compare May 21, 2020 14:04
@zachferland zachferland force-pushed the feat/external-provider branch from ddf2048 to 12d9e9d Compare May 22, 2020 19:40
@zachferland
Copy link
Contributor Author

Remaining:

  • bug when denying signature (errors not propagated correclty)
  • version iframe, since last release minimally deployed, can break with upgrade, but cant break existing deployments by upgrading the iframe here

@zachferland zachferland force-pushed the feat/external-provider branch from fb41d37 to 6cb9064 Compare May 25, 2020 16:15
@zachferland
Copy link
Contributor Author

@oed unfortunately not ready to merge yet, found issue where getting "Could not append entry, key ..." on first space auth, but not on following auth, didnt see earlier, appears to be regression, but didnt find which change yet, ran out to time to fix today, will continue in morning

@zachferland zachferland requested a review from oed May 28, 2020 17:42
@zachferland zachferland force-pushed the feat/external-provider branch from 3513499 to a7a1d24 Compare May 28, 2020 18:08
Copy link
Member

@oed oed 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. Do you think it makes sense to get CAIP-10 in now, or wait until full migration. We also have upcoming work to support CAIP-10 in blockchain-utils next sprint.

Comment on lines +89 to +91
# filters:
# branches:
# only: develop
Copy link
Member

Choose a reason for hiding this comment

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

Should these be commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for now, just wanted branches deployed to dev

window.getProvider = (address) => {
return store.get(`provider_${address}`)
}
const mobileRegex = /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i
Copy link
Member

Choose a reason for hiding this comment

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

I assume you tested this on your BlackBerry 😝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup and my windows phone :) this is left over from before, reminded me, will probs switch this to a lib to handle

package.json Outdated
Comment on lines 40 to 42
"3id-blockchain-utils": "github:ceramicnetwork/js-3id-blockchain-utils#feat/ethereum-auth-function",
"@babel/runtime": "^7.1.2",
"@portis/web3": "^2.0.0-beta.54",
"@walletconnect/web3-provider": "^1.0.0-beta.47",
"authereum": "0.0.4-beta.129",
"fortmatic": "^2.0.5",
"identity-wallet": "^1.2.0",
"identity-wallet": "github:3box/identity-wallet-js#feat/3id-connect-v2",
Copy link
Member

Choose a reason for hiding this comment

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

These should be merged/released before this is merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, today

* @param {String} accountId Id of account used with provider, most often hex address
* @return {String} A 32-64 bytes hex string
*/
async authenticate(message, accountId) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be a CAIP-10 accountId right?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah thats the plan after 👍 wont matter here after, just getting passed through, but will in other places

* @param {String} accountId Id of account used with provider, most often hex address
* @return Returns on success
*/
async createLink(did, accountId) {
Copy link
Member

Choose a reason for hiding this comment

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

CAIP-10 here as well?

@@ -1,13 +1,18 @@
import ThreeIdProviderProxy from './threeIdProviderProxy.js'
import { expose } from 'postmsg-rpc'
import EthereumAuthProvider from './authProvider/ethereumAuthProvider.js'
import { fakeIpfs } from 'identity-wallet/lib/utils'
Copy link
Member

Choose a reason for hiding this comment

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

Looks like fakeIpfs isn't used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup recently, gonna remove

}
return this._threeId
_write3idState(state, address) {
store.set(serializedKey(address), state)
Copy link
Member

Choose a reason for hiding this comment

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

We should probably use CAIP-10 accountId here as well.

Comment on lines +97 to +101
async migration(spaces, address) {
const threeId = await this._getThreeId(address)
await threeId.authenticate(spaces)
return threeId.serializeState()
}
Copy link
Member

Choose a reason for hiding this comment

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

So the reason we are using the ThreeId class here is because we need the logic that uses the Keyring class thats in 3box-js. If I understand this correctly than the keyring code should no longer be needed in 3box-js right?

Actually I think we don't even need the keyring class anymore. The main thing we need is openSpaceConsent and openBoxConsent. Am I missing something here?
If not this should be in scope for the full migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the keyring and really a lot of the 3id class currently in 3boxjs wont be needed anymore, and yup plan is to refactor in the future

there is a bit more logic (simple though, like serializing, creating all keyrings for spaces, etc), and its nice to reuse to code (including current keyring) that has been working for a while, but yeah could be pulled out as migration module or something as part of refactor

@zachferland
Copy link
Contributor Author

Some more context, this assume eth providers still, but yeah shortly will generalize to any (some parts here), but will add docs on adding support (similar to before) but now in this repo, will add provider detection so should be able to detect providers and map to provided implementation (will be able to pass any supported provider without any extra work in 3boxjs), CAIP-10 account ids instead of addresses

@zachferland zachferland force-pushed the feat/external-provider branch from a995efb to 9515c12 Compare May 29, 2020 19:59
@zachferland zachferland force-pushed the feat/external-provider branch from 9515c12 to a74b528 Compare May 30, 2020 17:36
@zachferland zachferland merged commit b0184e5 into develop May 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants