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

Connection should be able to take materialized invocations #195

Closed
Gozala opened this issue Jan 11, 2023 · 0 comments · Fixed by #199
Closed

Connection should be able to take materialized invocations #195

Gozala opened this issue Jan 11, 2023 · 0 comments · Fixed by #199

Comments

@Gozala
Copy link
Collaborator

Gozala commented Jan 11, 2023

I was telling @gobengo that we could route request from one server to the other simply by passing them to connection.execute, but turns out this was incorrect because:

  1. Connection's execute takes IssuedInvocation (Well tuple of ServiceInvocations but those are basically IssuedInvocations) which is not an Delegation it's like a lazy version that can be materialized as needed

    https://github.com/web3-storage/ucanto/blob/582c4c504d2e75feee2c5d298ea08b2f01ff1c5e/packages/interface/src/lib.ts#L232-L239

  2. Connection passes these lazy invocations into encoder to encode

    https://github.com/web3-storage/ucanto/blob/582c4c504d2e75feee2c5d298ea08b2f01ff1c5e/packages/client/src/connection.js#L47-L49

  3. Which in turn create actual delegations by materializing them

    https://github.com/web3-storage/ucanto/blob/582c4c504d2e75feee2c5d298ea08b2f01ff1c5e/packages/transport/src/car.js#L19-L23

Few things to note here:

  1. For a while I have been considering to get rid off these lazy invocation things which would make .invoke and .delegate functions more similar. I'm still hesitant however as they introduce additional awaits, first you await to create invocation then you await to execute the invocation.
  2. Even with lazy invocations I think it is a mistake to make materializing them transport's problem. It would make a lot more sense to make execute materialize all of the invocations first and then pass those onto transport to just do encoding.
    • It probably ended up in transport because there was a loop iterating invocations and I just avoided second one

Fix

Option 1

  • I think we should change transport API so it expects materialized invocations and make materializing them connection's problem
  • We could also loosen up execute function to allow passing both materalized and non-materialized invocations so it could materialize ones that aren't yet and forward them onto transport

Option 2

Perhaps whole materialized / not-yet-materalized invocations could be abstracted away with some EncodableInvocation interface of sorts that provides some method e.g. .delegate() which returns a materialized invocation. That way connection, transport and all those layers could just expect EncodableInvocation and not care which one they're actually getting.

Other than type changes it would basically mean adding delegate() { return this } to the Delegation class because Invocation already has .delegate()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant