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

Add ERC: Public Cross Port #62

Merged
merged 33 commits into from Feb 2, 2024
Merged

Add ERC: Public Cross Port #62

merged 33 commits into from Feb 2, 2024

Conversation

JXRow
Copy link
Contributor

@JXRow JXRow commented Oct 30, 2023

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

@eip-review-bot
Copy link
Collaborator

eip-review-bot commented Oct 30, 2023

✅ All reviewers have approved.

@eip-review-bot eip-review-bot changed the title Add ERC: Public Cross Port Add EIP: Public Cross Port Oct 30, 2023
@github-actions github-actions bot added the w-ci label Oct 30, 2023
@eip-review-bot eip-review-bot changed the title Add EIP: Public Cross Port Website: Public Cross Port Oct 30, 2023
@github-actions github-actions bot added w-ci and removed w-ci labels Oct 30, 2023
@JXRow JXRow changed the title Website: Public Cross Port Add ERC: Public Cross Port Oct 30, 2023
@JXRow JXRow changed the title Add ERC: Public Cross Port Add ERC-7533: Public Cross Port Oct 30, 2023
@eip-review-bot eip-review-bot changed the title Add ERC-7533: Public Cross Port Add ERC: Public Cross Port Oct 31, 2023
Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

ERCS/erc-7533.md Outdated Show resolved Hide resolved
ERCS/erc-7533.md Outdated Show resolved Hide resolved
ERCS/erc-7533.md Outdated Show resolved Hide resolved
ERCS/erc-7533.md Outdated Show resolved Hide resolved
ERCS/erc-7533.md Show resolved Hide resolved
ERCS/erc-7533.md Outdated Show resolved Hide resolved
ERCS/erc-7533.md Outdated Show resolved Hide resolved
assets/erc-7533/0.png Outdated Show resolved Hide resolved
@github-actions github-actions bot added the w-ci label Jan 18, 2024
Copy link

The commit a2c3045 (as a parent of 5bc9997) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot removed the w-ci label Jan 18, 2024
Copy link
Collaborator

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

This is looking good so far!

My last large bit of feedback is that you should move the description of how the interfaces are supposed to be used (the "Add cross-chain message", "Pull roots & Set roots", and "Verify cross-chain message" sections) into the Specification section.

The Specification section alone should give the reader enough information to implement/use your proposal. The Rationale section is there to give your reasoning behind why you made certain choices within the proposal.

For example, this paragraph:

Upon the completion of packing a new MerkleTree, the package carrier (usually the cross-chain bridge project) pulls the root from multiple chains and stores it in the IReceivePort contract of each chain. It is important to note that the traditional approach involves using a push method, as depicted in the following diagram:

Is describing what the package carrier is supposed to be doing. That kind of content belongs in Specification.

Conversely, this paragraph is a great example of something that should stay in Rationale:

The MerkleTree data structure efficiently compresses the size of cross-chain messages. Regardless of the number of cross-chain messages, they can be compressed into a single root, represented as a byte32 value. The package carrier only needs to transport the root, resulting in low gas cost.

ERCS/erc-7533.md Outdated Show resolved Hide resolved
ERCS/erc-7533.md Outdated Show resolved Hide resolved

This EIP does not change the consensus layer, so there are no backwards compatibility issues for Ethereum as a whole.

This EIP does not change other ERC standars, so there are no backwards compatibility issues for Ethereum applications.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This EIP does not change other ERC standars, so there are no backwards compatibility issues for Ethereum applications.
This EIP does not change other ERC standards, so there are no backwards compatibility issues for Ethereum applications.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still showing as standars for me, which isn't a word.

---
eip: 7533
title: Public Cross Port
description: Help bridges to connect all EVM chains
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think of:

Suggested change
description: Help bridges to connect all EVM chains
description: Pull-based messaging for cross chain communication

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still not sure the current description is descriptive enough. What differentiates this proposal from other bridges? What makes Public Cross Ports special?

@JXRow JXRow requested a review from SamWilsn January 19, 2024 19:32
JXRow and others added 9 commits January 20, 2024 03:36
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
It's not specification, but developers need to know the security suggestions
Copy link
Collaborator

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

Looks acceptable for a draft. Please fix the typo and expand your description at your leisure.

---
eip: 7533
title: Public Cross Port
description: Help bridges to connect all EVM chains
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still not sure the current description is descriptive enough. What differentiates this proposal from other bridges? What makes Public Cross Ports special?


This EIP does not change the consensus layer, so there are no backwards compatibility issues for Ethereum as a whole.

This EIP does not change other ERC standars, so there are no backwards compatibility issues for Ethereum applications.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still showing as standars for me, which isn't a word.

@eip-review-bot eip-review-bot enabled auto-merge (squash) February 2, 2024 19:54
Copy link
Collaborator

@eip-review-bot eip-review-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eip-review-bot eip-review-bot merged commit 9bfa355 into ethereum:master Feb 2, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants