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 docs/sequences.md with deposit, withdraw, and exit sequences #1354

Merged
merged 13 commits into from
Apr 4, 2023

Conversation

sergerad
Copy link
Contributor

@sergerad sergerad commented Apr 2, 2023

Description

Suggesting to add sequence diagrams under docs/ section.

The diagrams render on github so they could be very helpful. See https://github.com/0xPolygon/polygon-edge/blob/424ae7fb723365c0ef245fd731f76645e2920d92/docs/sequences.md

I have been using these when I am debugging deployments as the flows are quite complicated.

Limitations

The sequences there are based on my understanding and likely contain errors and lack some important information.

If Polygon team thinks these would be helpful to others, please contribute to this PR.

@sergerad
Copy link
Contributor Author

sergerad commented Apr 2, 2023

@Stefan-Ethernal please consider whether you would contribute to this PR to help myself and others understand the system better.

Link to rendered diagrams:
https://github.com/0xPolygon/polygon-edge/blob/424ae7fb723365c0ef245fd731f76645e2920d92/docs/sequences.md

Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal left a comment

Choose a reason for hiding this comment

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

Nice one and pretty useful. Thanks for your contribution 🙌
Leaving a couple of comments to consider.

Btw, maybe move the file itself to docs/bridge/sequences.md.

docs/sequences.md Outdated Show resolved Hide resolved
docs/sequences.md Outdated Show resolved Hide resolved
docs/sequences.md Outdated Show resolved Hide resolved
docs/sequences.md Outdated Show resolved Hide resolved
docs/sequences.md Outdated Show resolved Hide resolved
docs/sequences.md Outdated Show resolved Hide resolved
docs/sequences.md Outdated Show resolved Hide resolved
sergerad and others added 9 commits April 3, 2023 21:06
Co-authored-by: Stefan Negovanović <93934272+Stefan-Ethernal@users.noreply.github.com>
Co-authored-by: Stefan Negovanović <93934272+Stefan-Ethernal@users.noreply.github.com>
Co-authored-by: Stefan Negovanović <93934272+Stefan-Ethernal@users.noreply.github.com>
Co-authored-by: Stefan Negovanović <93934272+Stefan-Ethernal@users.noreply.github.com>
@sergerad
Copy link
Contributor Author

sergerad commented Apr 3, 2023

Thanks @Stefan-Ethernal, appreciate it. Have made the changes you suggested.
https://github.com/0xPolygon/polygon-edge/blob/bb52c352859dae158ad4b25cb17e141de8b16af8/docs/bridge/sequences.md

One thing I am still confused about is at what point in the withdraw sequence child native tokens should be transferred out of an address. I am observing the following on my deployment:

  1. The following commands succeed:
# deposit 1000 wei
polygon-edge bridge deposit-erc20 ...
# withdraw 1000 wei
polygon-edge bridge withdraw-erc20 ...

polygon-edge bridge 'exit' ...
  1. Using eth_getBalance JSON RPC request via Postman after exit shows us the balance is still 1000 wei.

This is for native token. Am I missing something with the ChlidERC20Predicate.sol burn() etc? Thanks!

@Stefan-Ethernal
Copy link
Collaborator

Stefan-Ethernal commented Apr 3, 2023

One thing I am still confused about is at what point in the withdraw sequence child native tokens should be transferred out of an address. I am observing the following on my deployment:

  1. The following commands succeed:
# deposit 1000 wei
polygon-edge bridge deposit-erc20 ...
# withdraw 1000 wei
polygon-edge bridge withdraw-erc20 ...

polygon-edge bridge 'exit' ...
  1. Using eth_getBalance JSON RPC request via Postman after exit shows us the balance is still 1000 wei.

This is for native token. Am I missing something with the ChlidERC20Predicate.sol burn() etc? Thanks!

As soon as withdraw-erc20 is called, a specified amount is burnt on the child chain (by the ChildERC20Predicate: https://github.com/0xPolygon/core-contracts/blob/dev/contracts/child/ChildERC20Predicate.sol#L135).

Are you sure that you are querying the correct account address (the private key which is provided when sending the withdraw transaction must be the one you are querying eth_getBalance)? Other than that I cannot think of any reason why it wouldn't reduce its balance. I can try to test it once again on my end with some EOA (because until now, we were using some of the validator's accounts to send withdrawal transactions).

Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@Stefan-Ethernal Stefan-Ethernal requested a review from a team April 3, 2023 10:13
Copy link
Collaborator

@goran-ethernal goran-ethernal left a comment

Choose a reason for hiding this comment

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

Apart from the small comment, this is great. Hats down to you good sir 🎩

Co-authored-by: Goran Rojovic <100121253+goran-ethernal@users.noreply.github.com>
@sergerad
Copy link
Contributor Author

sergerad commented Apr 3, 2023

(because until now, we were using some of the validator's accounts to send withdrawal transactions).

Thanks, this clarified things for me

@Stefan-Ethernal Stefan-Ethernal merged commit 0972756 into 0xPolygon:develop Apr 4, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants