fix: call claimAccess at the end of authorizeWithPollClaim

[travis]

I'm testing out using authorizeWithPollClaim instead of authorizeWithSocket to see if it will return more quickly after the email validation link is clicked and therefore better for demos. It worked, but doesn't currently run claimDelegations and therefore doesn't infer spaces from the list of claimed delegations.

This feels a bit duplicative because we're already executing access/claim several times while we poll, but one more time won't hurt much and this makes it more consistent with authorizeWithSocket.

[gobengo]

Can you either define this as a new thing, or else wherever you are tempted to use this, do authorizeWithPollClaim().then(() => claimAccess(...))

[travis]

I have a strong preference for authorizeWithPollClaim and authorizeWithSocket both behaving the same way here - either they should both automatically claimAccess or both not do that. Since authorizeWithSocket is already in use in both clients and authorizeWithPollClaim is unused I'd like to stick with authorizeWithSocket's semantics for now.

I know you would like to simplify this layer and push responsibility for calling claimAccess up to the clients and I'm generally supportive of that, but I think we should open an issue for that refactoring and address it once we're out of crunch mode.

wdyt?

[travis]

fwiw I'm also open to defining a new thing, but I think it should be named differently - ie, I think authorize{WithSocket|WithPollClaim} should behave the same way - if you'd like to preserve a use case that doesn't claimAccess that's fine but lets call it something different to distinguish it from the existing functionality.

[travis]

one more thought on this - I could call claimAccess in react-keyring/src/providers/Keyring.tsx but fwiw if feels awkward to be dealing with lower level details like the issuer DID and the addProofs option at that level.

In other words, I could go from this:

      await authorizeWithPollClaim(agent, email, { signal: controller.signal })

to this:

      await authorizeWithPollClaim(agent, email, { signal: controller.signal })
      await accessClaim(agent, agent.issuer.did(), { addProofs: true })

but that gets this React module in the business of thinking about issuer DIDs and whether it should "add proofs" - I'd rather encapsulate those details down here in the use cases, which suggests to me that we should have some functions in agent-use-cases that do what I'm doing for both the websocket and polling cases no matter what.

[gobengo]

(update: originally I thought you were adding this function to expectNewClaimableDelegations vs this one. I mind much less on this one, but tbh I'd rather just not even export it at all and have it defined at the layer you are trying to use it from)

what's important to me is that we maintain there being a way of calling authorizeWithPollClaim that does not also do more stuff than that. That way you can compose it with other things. What I don't like about adding more functionality to the function that was there is that it makes the function that was there no longer useful for the case where you don't want to claim+addProofs.

If you want there to be a function that does it and then also claims and add proofs, it doesn't seem like it needs to be defined in this library (it wasn't needed until now). If it were defined in this library, I'd want there to be a test that verifies that it does the new claim+add bits in the way you expect it to, so if someone sees this function and is like 'why does this function that's called 'authorizeWith...' or 'authorizeAndWait' do more than just invoking access/authorize and waiting?', and remove the claim+addProofs, a test fails

I have a strong preference for authorizeWithPollClaim and authorizeWithSocket both behaving the same way here - either they should both automatically claimAccess or both not do that.

I want this too, it's just that I would make them consistent by removing claim+add from both.
tbh I'd prefer neither authorizeWithPollClaim nor authorizeWithSocket are exported from the package.

with all that said, since I realized your only touching the authorizeWithPollClaim (which I didn't anticipate anyone would use), making them consistent the way you want isn't something I block. it just seems like a change that test should require

[gobengo]

I could call claimAccess in react-keyring/src/providers/Keyring.tsx but fwiw if feels awkward to be dealing with lower level details like the issuer DID and the addProofs option at that level.

so dont do it at that level, but also don't do it at this level.
there is the w3up-client level storacha/w3ui#295

[alanshaw]

Yes lets remove the side effect from authorizeWithSocket and instead export a authorize function that doesn't expose the implementation details.

The w3up-client authorize function at the top level should add this sugar, i.e. do both authorize + claim and the lower level api capability.access.authorize and capability.access.claim does it separately.

[travis]

Yes lets remove the side effect from authorizeWithSocket and instead export a authorize function that doesn't expose the implementation details.

Hm, the intention tho was for authorizeWithSocket and authorizeWithClaimPoll to be drop-in replacements for the old authorize function that use different mechanisms for waiting for the email link to be clicked.

I'm in favor of having a version of these functions that don't do the claim, but would like to keep the current names for backwards compatibility at least in the short term, so maybe something like:

authorizeWithSocketNoClaim # executes `access/authorize`, waits on socket
authorizeWithSocket # executes `access/authorize`, waits on socket, executes `access/claim`
authorizeWithPollClaimNoClaim # executes `access/authorize`, waits on socket
authorizeWithPollClaim # executes `access/authorize`, waits on socket, executes `access/claim`

I can add big scary comments that talk about the intended future direction here - the names are ugly but honestly that just makes me more confident we'll refactor this soon.

Also worth mentioning - after chatting in Colo this morning I'm inclined to just change the KV polling interval here:

https://github.com/web3-storage/w3protocol/blob/main/packages/access-api/src/routes/validate-ws.js#L39

So I may close this PR for now anyway!

[gobengo]

So I may close this PR for now anyway!

I think there was an element of your motivation to file this which was that authorizeWithPollClaim doesn't do what you'd expect it to.
tbh I threw that in just to illustrate that it was possible, and I didn't really realize/expect people would export it from the package and use in other libraries.

Is it okay with you if I remove authorizeWithPollClaim it from package exports? (others are free to define something similar in their libraries, and customize it according to their requirements)

(related, this is why I have a rule to never write export * from, but I break my own rule this one time :P and now this happens)

[travis]

Is it okay with you if I remove authorizeWithPollClaim it from package exports? (others are free to define something similar in their libraries, and customize it according to their requirements)

works for me!