abcoathup GSN Bouncers review #1916
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
frangio
reviewed
Sep 16, 2019
There was a problem hiding this comment.
Thanks @abcoathup! There are some good improvements here. I think the GSNBouncerERC20Fee section could use some restructuring of the content. I left some details in a comment below.
|
|
||
| The only thing you must do is extend from `GSNRecipient` and implement the accept method. | ||
| Depending on the logic for your Custom Bouncer, you may need to implement `_postRelayedCall` and `_preRelayedCall`. |
There was a problem hiding this comment.
I think we should explain how the charge estimates work, and why they work that way: that pre will tell you the maximum possible charge, and later post will tell you the real amount. Or point towards GSNBouncerERC20Fee as an example of how to deal with it.
There was a problem hiding this comment.
I have pointed to the GitHub repo 2.4.0 versions of GSNBouncerERC20Fee and GSNBouncerSignature. Is there a better way to do this?
Co-Authored-By: Francisco Giordano <[email protected]>
Co-Authored-By: Francisco Giordano <[email protected]>
|
WIP, still a couple of points to handle tomorrow. |
|
Updated, ready for review |
frangio
reviewed
Sep 25, 2019
Co-Authored-By: Francisco Giordano <[email protected]>
frangio
approved these changes
Sep 25, 2019
frangio
pushed a commit
that referenced
this pull request
Oct 8, 2019
* Fix typo * Replace pseudo code contracts with sample code * Update GSN Bouncers text * More text changes * Update with latest code and remove reference to allowance * Capitalize Custom Bouncer * Update docs/modules/ROOT/pages/gsn-bouncers.adoc Co-Authored-By: Francisco Giordano <[email protected]> * Update gsn-bouncers.adoc with Antora cross reference Co-Authored-By: Francisco Giordano <[email protected]> * Revert to handling msg.sender msg.data differently * Change by default to simplest implementation * Change signing to include signature for GSNBouncerSignature * Reword summary of what is in the guide * Remove "The" from before `GSNBouncer...` * Fix code snippet markdown * Change to API references to xref:api * Remove code from How it works sections * Explain 1:1 exchange rate * Change transaction request to relayed call * Minor fixes * Add info to Custom Bouncers * Typo * Minor fixes * reorder sentence based on review gsn-bouncers.adoc Co-Authored-By: Francisco Giordano <[email protected]> * Improve wording of signing of relayed call parameters by trusted signer (cherry picked from commit 18473d0)
frangio
pushed a commit
that referenced
this pull request
Oct 29, 2019
* Fix typo * Replace pseudo code contracts with sample code * Update GSN Bouncers text * More text changes * Update with latest code and remove reference to allowance * Capitalize Custom Bouncer * Update docs/modules/ROOT/pages/gsn-bouncers.adoc Co-Authored-By: Francisco Giordano <[email protected]> * Update gsn-bouncers.adoc with Antora cross reference Co-Authored-By: Francisco Giordano <[email protected]> * Revert to handling msg.sender msg.data differently * Change by default to simplest implementation * Change signing to include signature for GSNBouncerSignature * Reword summary of what is in the guide * Remove "The" from before `GSNBouncer...` * Fix code snippet markdown * Change to API references to xref:api * Remove code from How it works sections * Explain 1:1 exchange rate * Change transaction request to relayed call * Minor fixes * Add info to Custom Bouncers * Typo * Minor fixes * reorder sentence based on review gsn-bouncers.adoc Co-Authored-By: Francisco Giordano <[email protected]> * Improve wording of signing of relayed call parameters by trusted signer (cherry picked from commit 18473d0)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have updated the document based on my review.
I used request transactions rather than call, though the code uses call.