-
Notifications
You must be signed in to change notification settings - Fork 23
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
Cleanup docs #87
Cleanup docs #87
Conversation
Signed-off-by: cam-schultz <[email protected]>
* Requirements: | ||
* | ||
* - `message` must have been previously sent to the given destination chain ID. | ||
* - `message` must have been previously sent to the given `destinationChainID`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should rename this to destinationBlockchainID
, as per our other discussions of confusing this with the ethereum chainID
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will echo this as DevRel brought it up too. They also suggsted possible avalancheChainID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely agree that we should rename these variables to destinationBlockchainID
. Slight preference for that over destinationAvalancheChainID
for verbosity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, I think this is best suited for a standalone set of PRs, since it will require coordination on the awm-relayer side. Ticket: #95
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm thanks for making the issues
|
||
Create a state variable of `ITeleporterMessenger` type called `teleporterMessenger`. Then we'll create a constructor for our contract that takes in an address where the Teleporter messenger would be deployed on this chain, and set our state variable with it. | ||
Create a state variable of `ITeleporterMessenger` type called `teleporterMessenger`. Then create a constructor for our contract that takes in an address where the Teleporter messenger would be deployed on this chain, and set our state variable with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
above we have diferent capitalizatoin with "Teleporter Messenger protocol", how do we want to standardize this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My opinion is that we should always capitalize Teleporter and Teleporter Messenger, since they refer to distinct entities (the Teleporter protocol and the TeleporterMessenger contract)
@@ -35,11 +35,11 @@ struct TeleporterFeeInfo { | |||
} | |||
|
|||
/** | |||
* @dev Interface that describes functionalities for a cross chain messenger. | |||
* @dev Interface that describes functionalities for a cross-chain messenger implementing the Teleporter protcol. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have some references to "Teleporter Messenger protocol", should unify these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I think just calling it Teleporter is more appropriate. "Teleporter Messenger" is an implementation of the Teleporter protocol. This interface is specifically tied to "Teleporter Messenger", but I think we should consider paring it back to conform only the Teleporter protocol. All the additional methods included in TeleporterMessenger.sol
would be outside of the protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think it makes sense that when we refer to "protocol" it refers to Teleporter protocol only? Like you said Teleporter Messenger is an implementation of the protocol, so in the READMEs we can remove all references to Teleproter Messenger as a protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Replaced "Teleporter Messenger protocol" with "Teleporter protocol" in a few places. Lmk if you spot any others.
Co-authored-by: F. Eugene Aumson <[email protected]> Signed-off-by: cam-schultz <[email protected]>
Co-authored-by: F. Eugene Aumson <[email protected]> Signed-off-by: cam-schultz <[email protected]>
Co-authored-by: F. Eugene Aumson <[email protected]> Signed-off-by: cam-schultz <[email protected]>
Co-authored-by: F. Eugene Aumson <[email protected]> Signed-off-by: cam-schultz <[email protected]>
Co-authored-by: F. Eugene Aumson <[email protected]> Signed-off-by: cam-schultz <[email protected]>
Co-authored-by: F. Eugene Aumson <[email protected]> Signed-off-by: cam-schultz <[email protected]>
Co-authored-by: F. Eugene Aumson <[email protected]> Signed-off-by: cam-schultz <[email protected]>
Co-authored-by: F. Eugene Aumson <[email protected]> Signed-off-by: cam-schultz <[email protected]>
Co-authored-by: F. Eugene Aumson <[email protected]> Signed-off-by: cam-schultz <[email protected]>
Co-authored-by: F. Eugene Aumson <[email protected]> Signed-off-by: cam-schultz <[email protected]>
Co-authored-by: F. Eugene Aumson <[email protected]> Signed-off-by: cam-schultz <[email protected]>
Co-authored-by: F. Eugene Aumson <[email protected]> Signed-off-by: cam-schultz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks for cleaning up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requested one more small change here: #87 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Why this should be merged
Brings the Teleporter READMEs up-to-date with the latest implementation.
Also moves a handful of scripts from
scripts/local/
toscripts/