feat: add an asv2 resolver #143

silasbw · 2022-12-19T16:15:49Z

No description provided.

silasbw · 2022-12-19T16:19:30Z

src/resolve-celo.ts

+  }: {
+    providerUrl: string
+    federatedAttestationsProxyContractAddress: Address
+    authSigner: AuthSigner


I'd like to avoid exposing types of AuthSinger and ServiceContext.

In general, it seems prudent to avoid exposing specific implementation details of how we might resolve things. E.g., it shouldn't matter to the caller of this library if we use web3.js or ethers; or the ODIS SDK or something else; etc.

We could check with the identity team if we can avoid exposing this. E.g., is it OK with them if we manually construct a ServiceContext from constants vs using the SDK helpers?

Totally fine to declare your own ServiceContext constant, but using the getter in the SDK ensures your info is the most up to date.

I'm a little confused around your point about exposing how things are resolved. You can use the ODIS SDK with web3js or ethers already, and you can even forgo the SDK and make queries directly to ODIS if you really wanted to

I don't want to expose the implementation details of resolve-kit to the caller. E.g., if resolve-kit uses web3js, but then wants to change to ethers, the users of this library shouldn't notice.

re ServiceContext: that's kinda my question. Do you want to manage the specific constants via the SDK? or are you OK if users (i.e., Valora) use them directly? I think those two options have different maintenance implications on the ODIS SDK.

@silasbw if I'm understanding this correctly, you don't want the caller of this library to need to pass in arguments of types ServiceContext and AuthSigner since those types are defined within the identity sdk, right?
I think it should work fine to just pass in a private key (instead of AuthSigner) and environment name (i.e. 'mainnet' or 'alfajores', instead of ServiceContext) and construct the ServiceContext and AuthSigner to pass in to the identity sdk within the ResolveCelo constuctor.
Does that address your concern?

Kinda. We try (with varying degrees of success) to avoid interfaces that take a specific network name, and instead try to use the values directly that are connoted by that network name (e.g., ~~alfajores~~ -> https://alfajores-forno.celo-testnet.org; or ~~local~~ -> http://localhost:7545). We hope this facilitates more controlled integration tests, but we make compromises all the time and it sounds non-trival to spin up an ODIS instance, so what you suggest is fine.

did you decide to keep these parameters as type ServiceContext and AuthSigner?

Yes, for now :/, if that's OK.

codecov-commenter · 2022-12-19T18:18:27Z

Codecov Report

Merging #143 (da77830) into main (c7ebb72) will increase coverage by 5.26%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
+ Coverage   87.23%   92.50%   +5.26%     
==========================================
  Files           4        5       +1     
  Lines          47       80      +33     
  Branches        7        9       +2     
==========================================
+ Hits           41       74      +33     
  Misses          6        6

Impacted Files	Coverage Δ
src/resolve-celo.ts	`100.00% <100.00%> (ø)`
src/types.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7ebb72...da77830. Read the comment docs.

isabellewei · 2022-12-19T21:46:42Z

src/resolve-celo.ts

+
+  async resolve(id: string): Promise<NameResolutionResults> {
+    try {
+      // TODO(sbw): need a timeout


you can just wrap the getObfuscatedIdentifier function in a timeout function timeout(OdisUtils.Identifier.getObfuscatedIdentifier, [params], timeoutMs, ...)

I would just note that the getObfuscatedIdentifier function retries the ODIS query if it returns an error, so the timeout would consider all the retries as one attempt

...this might be dumb question: what's the timeout function?

...I found a reference 1 in the celo-monorepo. It looks like timouting this way during long-lived connections could cause cascading issues, like a bunch of open sockets building up. We'd like to avoid that if possible.

from the slack thread surrounding this, it seems like we need to update either @celo/base or @celo/identity to take in an AbortController. We've created a ticket for this here and will work on implementing it ASAP - most likely in the new year celo-org/celo-monorepo#10083

updating that the above ticket is done ^

isabellewei · 2022-12-19T21:48:29Z

src/resolve-celo.ts

+  }: {
+    providerUrl: string
+    federatedAttestationsProxyContractAddress: Address
+    authSigner: AuthSigner


I'm a little confused around your point about exposing how things are resolved. You can use the ODIS SDK with web3js or ethers already, and you can even forgo the SDK and make queries directly to ODIS if you really wanted to

src/resolve-celo.ts

alecps

Looks great!! Only comment is regarding the trusted issuer list and making it easier to extend to allow for cross-wallet lookups

alecps · 2023-02-21T16:23:50Z

package.json

@@ -39,8 +39,9 @@
    "yargs": "^17.7.0"
  },
  "dependencies": {
-    "@celo/base": "^3.2.0",
-    "@celo/utils": "^3.2.0",
+    "@celo/base": "^3.0.1",


Is there a reason these aren't on the latest version?

alecps · 2023-02-21T16:27:20Z

src/index.ts

@@ -2,3 +2,4 @@ export * from './types'
 export { ResolveAddress } from './resolve-address'
 export { ResolveGroup } from './resolve-group'
 export { ResolveNom } from './resolve-nom'
+export { ResolveCelo } from './resolve-celo'


will this be updated to ResolveSocialConnect?

Yes, in a follow-on PR!

alecps · 2023-02-21T16:30:26Z

src/resolve-celo.ts

+  }: {
+    providerUrl: string
+    federatedAttestationsProxyContractAddress: Address
+    authSigner: AuthSigner


did you decide to keep these parameters as type ServiceContext and AuthSigner?

alecps · 2023-02-21T16:40:10Z

src/resolve-celo.ts

+      clearTimeout(timer)
+
+      const attestations = await this.federatedAttestationsContract.methods
+        .lookupAttestations(identifier, [this.account])


Note that if you only pass in the Valora issuer account here you will only be able to see attestations published by Valora. Even if you don't want to allow cross-wallet lookups now, it might make sense to add the option to pass a list of trusted issuers either to this function or to the constructor so it's easier in the future

This may also warrant adding a way to optionally store the issuer address on the NameResolution type

ohhh, what's the right way to pass in the current set of "suggested" trusted issuers?

Right now the set of issuers is so small that you can just hard code them in. We have the Kaala and Libera addresses listed in the docs here and can add Valora and Node's addresses as well. If the need arises in the future we have ideas about how to use a smart contract (possibly a token curated registry) to manage the "suggested" list of issuers

src/resolve-celo.ts

jeanregisser

Awesome! 👍

My main question about this is: what is the impact in Valora in the send flow?

Right now when entering a phone number this is what users see:

By adding an ASv2 resolution, this flow will then also show entries for the resolved address(es) for the entered number.
Which may be confusing.

If they click the entry with the phone number, it will use the normal internal resolution (central phone verification lookup + legacy decentralized lookup), possibly with secure send.

If they click the other pre-resolved entries, it will send to the resolved addresses.

I guess we should check what exact behavior we want so there's no surprise.

jeanregisser · 2023-03-08T14:16:42Z

src/resolve-celo.test.ts

+
+      const resolutions = await resolver.resolve('+18888888888')
+      expect(resolutions.resolutions.length).toBe(0)
+      expect(resolutions.errors.length).toBe(1)


Should we check the error itself here?

arthurgousset · 2023-03-08T17:44:16Z

Re @jeanregisser's point user flow point above, I thought I'd share a mockup of how Benoit and I designed the user flow in Kaala. This is how the Kaala production version in the AppStore currently handles lookups.

Co-authored-by: 0xarthurxyz <46296830+0xarthurxyz@users.noreply.github.com>

silasbw commented Dec 19, 2022

View reviewed changes

silasbw force-pushed the celo0 branch 2 times, most recently from 7c50233 to 62fe77c Compare December 19, 2022 18:16

isabellewei reviewed Dec 19, 2022

View reviewed changes

alecps reviewed Dec 21, 2022

View reviewed changes

src/resolve-celo.ts Show resolved Hide resolved

silasbw force-pushed the celo0 branch from 62fe77c to c3bfe9e Compare January 5, 2023 22:28

silasbw force-pushed the celo0 branch from 9d08916 to 9307d1c Compare February 21, 2023 16:18

alecps approved these changes Feb 21, 2023

View reviewed changes

silasbw force-pushed the celo0 branch from faa0978 to da77830 Compare March 4, 2023 03:51

arthurgousset reviewed Mar 7, 2023

View reviewed changes

src/resolve-celo.ts Outdated Show resolved Hide resolved

jeanregisser approved these changes Mar 8, 2023

View reviewed changes

silasbw and others added 7 commits March 14, 2023 09:18

feat: add an asv2 resolver

c5ab9f8

Update to use ODIS SDK timeout

3231268

Update doc link in comment

4b5df9c

Update

220157b

Fix rebase conflict resolutions

4570f84

Add a small set of trusted issuers

0046fa1

Update src/resolve-celo.ts

9e240e5

Co-authored-by: 0xarthurxyz <46296830+0xarthurxyz@users.noreply.github.com>

silasbw force-pushed the celo0 branch from 26cec19 to 9e240e5 Compare March 14, 2023 16:18

silasbw merged commit 1b35726 into main Mar 14, 2023

silasbw deleted the celo0 branch March 14, 2023 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add an asv2 resolver #143

feat: add an asv2 resolver #143

silasbw commented Dec 19, 2022

silasbw Dec 19, 2022

isabellewei Dec 19, 2022

isabellewei Dec 19, 2022

silasbw Dec 19, 2022

alecps Dec 21, 2022 •

edited

Loading

silasbw Dec 22, 2022

alecps Feb 21, 2023

silasbw Feb 22, 2023

codecov-commenter commented Dec 19, 2022 •

edited

Loading

isabellewei Dec 19, 2022 •

edited

Loading

silasbw Dec 19, 2022

silasbw Dec 19, 2022

alecps Dec 21, 2022

alecps Feb 21, 2023

isabellewei Dec 19, 2022

alecps left a comment

alecps Feb 21, 2023

alecps Feb 21, 2023

silasbw Feb 22, 2023

alecps Feb 21, 2023

alecps Feb 21, 2023

alecps Feb 21, 2023

silasbw Feb 22, 2023

alecps Feb 22, 2023 •

edited

Loading

jeanregisser left a comment

jeanregisser Mar 8, 2023

arthurgousset commented Mar 8, 2023 •

edited

Loading

feat: add an asv2 resolver #143

feat: add an asv2 resolver #143

Conversation

silasbw commented Dec 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecps Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 19, 2022 • edited Loading

Codecov Report

isabellewei Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecps Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

jeanregisser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthurgousset commented Mar 8, 2023 • edited Loading

alecps Dec 21, 2022 •

edited

Loading

codecov-commenter commented Dec 19, 2022 •

edited

Loading

isabellewei Dec 19, 2022 •

edited

Loading

alecps Feb 22, 2023 •

edited

Loading

arthurgousset commented Mar 8, 2023 •

edited

Loading