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

feat: draft wrap 0.2 specification #5

Draft
wants to merge 3 commits into
base: wrap-0.2-dev
Choose a base branch
from

Conversation

Niraj-Kamdar
Copy link
Contributor

No description provided.

spec/wrap.md Outdated
The Wasm module must define the following imports and exports:

#### Exports
- `_invoke: (bufferLen: u32) => u32`
Copy link
Contributor

Choose a reason for hiding this comment

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

What were your reasons for removing the "wrap" prefix? I do like the look of this, just curious. Maybe have a section in the doc that covers the considerations & design choices made from 0.1 -> 0.2, like we did in the ABI PR.

Choose a reason for hiding this comment

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

I'm torn, we can talk about it on the ACDC

spec/wrap.md Outdated

#### Exports
- `_invoke: (bufferLen: u32) => u32`
- sends the length of input buffer to Wasm module and it returns the length and the ptr to the new 8 byte long response buffer which contains length of the result buffer + pointer to the result buffer
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this sentence can be rewritten for better understanding. Maybe "The invoke export function receives the length of the invocation arguments buffer as its first and only argument. The invocation arguments buffer resides in an external host-controlled memory space, and must be copied into the receiving wasm module. To do this, the invoke export function's job is to allocate a new buffer of this size, and return a pointer to it, such that the host can copy the invocations arguments into it.

^^^ this might not be accurate, but you get my point, I think the wording of this paragraph can be massaged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: it doesn't return the buffer ptr, but instead it uses the __fill_buffer import fn.

Choose a reason for hiding this comment

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

Reworded this. The full explanation is in the invocation process below.

#### Imports
- `__subinvoke: (bufferPtr: u32, bufferLen: u32) => u32`
- Allows sending any buffer from the Wasm module to the host. Host returns the length of the result buffer to the Wasm module.
- `__fill_buffer: (bufferPtr: u32) => void`
Copy link
Contributor

Choose a reason for hiding this comment

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

This function feels too generic, and lacks context as to "what buffer is being filled". I think we could add a "bufferId" alongside the "bufferPtr" here. Also would need to add "bufferId" alongside the "bufferLen" in the _invoke export.

Copy link

@nerfZael nerfZael Feb 22, 2023

Choose a reason for hiding this comment

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

Reworded this a bit. But it is still a generic function, it can be either the invOptBuffer or subInvResBuffer that the host is sending to the wasm module. It doesn't need an ID to identify it because there can be only one type depending on context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function can also be named load_buffer
What do you guys prefer: fill_buffer 🍏 vs load_buffer 🍎

spec/wrap.md Outdated
The Wasm module must define the following imports and exports:

#### Exports
- `_invoke: (bufferLen: u32) => u32`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename "bufferLen" to "argsLen"? I think this buffer-use-case-specific naming is helpful for readers.

Copy link

@nerfZael nerfZael Feb 22, 2023

Choose a reason for hiding this comment

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

Renamed to optBufferLen to match the convention we have in the rest of the document.
It's the invoke options buffer length not the args buffer. Args buffer is inside of the invoke options as we defined below.

spec/wrap.md Outdated

#### Imports
- `__subinvoke: (bufferPtr: u32, bufferLen: u32) => u32`
- Allows sending any buffer from the Wasm module to the host. Host returns the length of the result buffer to the Wasm module.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be great to specify here that subinvokes are "calls to other wrap module functions, where the invocation method and arguments are packed into a buffer". Being specific about how the buffer is packed here is necessary IMO. Sure this could change, but that'd require a new minor version rev of WRAP (0.3 for ex).

Choose a reason for hiding this comment

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

Reworded this. It's still not calls to other wrap module functions, it's calls to the host functions.
We have the buffer structure defined below, these are just the export/import signatures and their descriptions.

@Niraj-Kamdar Niraj-Kamdar marked this pull request as draft February 21, 2023 13:29
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 this pull request may close these issues.

3 participants