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: txwrapper-template & CHAIN_BUILDER guide #35

Merged
merged 14 commits into from
Jan 4, 2021
Merged

Conversation

emostov
Copy link
Contributor

@emostov emostov commented Dec 26, 2020

Closes #9
Closes #33

What to review for:

  • Granularity of guide: I want it to be specific enough that there is no ambiguity in instructions but I don't want it to be so specific that any changes to the codebase will make parts of the guide irrelevant
  • Is advising examples overkill? My idea was that if each chain had its own examples they could easily show users how to handle any nuances. On the other hand it seems very repetitive with the example we already provide

Follow up work:

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Good stuff and thanks for the detailed docs! Left some nits and suggestions.

CHAIN_BUILDER.md Outdated
@@ -8,23 +8,118 @@ Creating a txwrapper package will expand the offline signing options for users o

## Note

This guide has very specific instructions on how to structure the public API of a chain's txwrapper package. This approach is taken by txwrapper-core's maintainers so users of existing txwrappers can quickly integrate with newly created txwrappers for `FRAME`-based chains. Feel free to open up a github issue to discuss any of these aspects.
This guide has very specific instructions on how to structure the public API of a chain's txwrapper package. This approach is taken by txwrapper-core's maintainers so users of existing txwrappers can quickly integrate with newly created txwrappers for `FRAME`-based chains. Feel free to open up a github issue in this repo to discuss any of these aspects.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This guide has very specific instructions on how to structure the public API of a chain's txwrapper package. This approach is taken by txwrapper-core's maintainers so users of existing txwrappers can quickly integrate with newly created txwrappers for `FRAME`-based chains. Feel free to open up a github issue in this repo to discuss any of these aspects.
This guide has very specific instructions on how to structure the public API of a chain's txwrapper package. This approach is taken by txwrapper-core's maintainers so users of existing txwrappers can quickly integrate with newly created txwrappers for `FRAME`-based chains. Feel free to open a github issue in this repo to discuss any of these aspects.

Copy link
Contributor

Choose a reason for hiding this comment

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

"This approach is taken by txwrapper-core's maintainers so users of existing txwrappers can quickly integrate with newly created txwrappers for FRAME-based chains." – this is a bit hard to read; not sure what to suggest here though. Maybe we can remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about "This approach is taken so existing txwrapper users can easily integrate new txwrappers."?

CHAIN_BUILDER.md Outdated Show resolved Hide resolved
CHAIN_BUILDER.md Outdated
```

3) **Choose relevant methods to re-export**
You will need to choose what pallet methods you want your txwrapper to expose. If you just need methods from Substrate or ORML pallets, checkout txwrapper-substrate and txwrapper-orml to see if the methods are already defined.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe link to the docs for txwrapper-substrate/txwrapper-orml here?

Do you think it would be a good idea to explain what kind of pallet methods it makes sense to export or do we expect the reader to come with that knowledge?

CHAIN_BUILDER.md Outdated Show resolved Hide resolved
CHAIN_BUILDER.md Outdated Show resolved Hide resolved
packages/txwrapper-template/examples/template-example.ts Outdated Show resolved Hide resolved
packages/txwrapper-template/examples/template-example.ts Outdated Show resolved Hide resolved
console.log(`\nExpected Tx Hash: ${expectedTxHash}`);

// Send the tx to the node. Again, since `txwrapper` is offline-only, this
// operation should be handled externally. Here, we just send a JSONRPC
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just log the tx or dump it to disk and end the script here and then add a separate script to submit the tx? To make it super-extra clear what the point of txwrapper is: prepare a transaction on one machine and send it off somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about being more explicit in the example with separating online/offline parts, but I think users need to design their own flow and this example is simply how to use the methods. For example, some flows may only use txwrapper to decode on the offline device (using another tool for sig generation), combining the signature with payload on an online device. I also think the example/README Offline vs Online section goes into enough detail

}

/**
* Signing function. Implement this on the OFFLINE signing device.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

packages/txwrapper-template/src/index.ts Outdated Show resolved Hide resolved
@emostov
Copy link
Contributor Author

emostov commented Dec 29, 2020

After addressing feedback, cleaning up some odds and ends, and rereading I think its looking good.

I would also like to make this repo public after this PR, thoughts @dvdplm ? EDIT We should also probably close #29 prior

@emostov emostov requested a review from dvdplm December 29, 2020 20:11
CHAIN_BUILDER.md Outdated Show resolved Hide resolved
Co-authored-by: Andrew Plaza <aplaza@liquidthink.net>
@emostov emostov merged commit 427ea8c into main Jan 4, 2021
@emostov emostov deleted the zeke-builder-guide branch January 4, 2021 17:47
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.

Create a template repo or directory How to guide for chain builders building their own txwrapper-* package
3 participants