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

Update xcm-format with v3 Instructions and Registers #31

Merged
merged 24 commits into from
Apr 12, 2023

Conversation

CrackTheCode016
Copy link
Contributor

@CrackTheCode016 CrackTheCode016 commented Mar 9, 2023

In an effort to sync, update, and expand the documentation around XCM in the Polkadot Wiki, I propose adding the newly added instructions and registers.

Most of the definitions for the instructions came from the XCM v3 commit code in the Polkadot repo.

Instructions Added:

  • ExpectAsset
  • ExpectError
  • ExpectOrigin
  • QueryPallet
  • ExpectPallet
  • ReportTransactStatus
  • ClearTransactStatus
  • LockAsset
  • UnlockAsset
  • NoteUnlockable
  • RequestUnlock

Registers Added:

  • Transact Status
  • Topic

TODO

  • MultiLocation
  • Junction
    • GeneralKey
    • network fields
  • MultiAsset/WildMultiAsset
  • max_assets removal
  • NetworkId
  • BodyId/BodyPart
  • Concept of UniversalLocation (for the GlobalConsensus Junction)
  • Errors
  • Go through XCMv3 PR and ensure that all changes are reflected in the spec.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
CrackTheCode016 and others added 2 commits March 9, 2023 18:15
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
@CrackTheCode016
Copy link
Contributor Author

CrackTheCode016 commented Mar 9, 2023

Thank you for catching that!! Let me know if there is anything else you think would be wise to include/changed.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
CrackTheCode016 and others added 4 commits March 10, 2023 19:53
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
README.md Outdated Show resolved Hide resolved
@KiChjang
Copy link
Contributor

For reference, the XCM v3 PR is here: paritytech/polkadot#4097, make sure that every new feature mentioned in the description there is reflected on this PR.

Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

Took a dive on the XCM v3 PR and diffed it here to add what was missing

Thank you for keeping the standard up-to-date with the code 🎉

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@gavofyork
Copy link
Contributor

gavofyork commented Mar 15, 2023

Junction also changed more than this diff would imply: GeneralKey and all items which contained a network field also changed.

@vstam1
Copy link
Contributor

vstam1 commented Mar 16, 2023

@gavofyork Currently we describe most of the errors of the xcvm instructions as fallible or infallible. Do we want to change these error descriptions to the actual errors mentioned in the 8 The types of error in XCM section based on the implementation in the xcm-executor?

@CrackTheCode016
Copy link
Contributor Author

CrackTheCode016 commented Mar 16, 2023

Maybe not relevant for this PR, but would including hyperlinks to the xcm code in the Polkadot repo be interesting? I feel it would bring the spec more cohesion.

i.e., to specific instructions, or interfaces like XcmContext etc

@gavofyork
Copy link
Contributor

@gavofyork Currently we describe most of the errors of the xcvm instructions as fallible or infallible. Do we want to change these error descriptions to the actual errors mentioned in the 8 The types of error in XCM section based on the implementation in the xcm-executor?

Yes - that could be helpful.

@gavofyork
Copy link
Contributor

Maybe not relevant for this PR, but would including hyperlinks to the xcm code in the Polkadot repo be interesting? I feel it would bring the spec more cohesion.

i.e., to specific instructions, or interfaces like XcmContext etc

Sounds sensible.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

Left some comments on error types

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated

Sets the Fees Mode Register.

- `jit_withdraw: bool`: The fees mode item; if set to `true` then fees for any instructions are withdrawn as needed using the same mechanism as `WithdrawAssets`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a good explanation, because naturally people will ask why we don't simply just use WithdrawAsset to do the job. And I don't believe it is true that any fees are eligible to be withdrawn -- only a select few instructions do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

This better?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@vstam1 vstam1 requested a review from KiChjang April 6, 2023 14:59
Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

This looks like it's good to go 🚀

README.md Outdated Show resolved Hide resolved
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
@KiChjang KiChjang merged commit 6193fa1 into polkadot-fellows:master Apr 12, 2023
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.

5 participants