-
Notifications
You must be signed in to change notification settings - Fork 2
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 parachain MultiLocation #1
Conversation
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.
Add X1(AccountId32)
); | ||
} | ||
|
||
function encodeMultiLocationSameConsensusAccountId32( |
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.
IMHO it should be X1(AccountId32)
:
Example: /AccountID32(0x234234)
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 👍
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 👍
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.
Looks great! Added a few minor comments.
public pure returns (bytes memory) { | ||
XCMEnums.XcmV3Junctions interiorJunctions = XCMEnums.XcmV3Junctions.X1; | ||
XCMEnums.XcmV3Junction firstJunction = XCMEnums.XcmV3Junction.Parachain; | ||
return abi.encodePacked( |
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.
Should rather use byte.concat, which was introduced for this purpose. The former abi.encodePacked
is discouraged for concatenating bytes.
BitcoinCore, | ||
BitcoinCash | ||
} | ||
} |
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.
needs newline 😄
// SPDX-License-Identifier: Apache-2.0 | ||
pragma solidity ^0.8.13; | ||
|
||
library XCMEnums { |
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.
This library is a bit unnecessary. Can define the enums at the toplevel like so:
// SPDX-License-Identifier: Apache-2.0
pragma solidity ^0.8.19;
enum XcmV3Junction {
Parachain,
AccountId32,
AccountIndex64,
AccountKey20,
PalletInstance,
GeneralIndex,
GeneralKey,
OnlyChild,
Plurality,
GlobalConsensus
}
function encodeU8(uint8 input) internal pure returns (bytes1) { | ||
return bytes1(input); | ||
} | ||
} |
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.
missing newline
|
||
contract MultiLocationEncoder { | ||
|
||
function encodeMultiLocationSameConsensusParachain( |
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.
Is a fairly verbose function name. Given that name of the contract suggests multilocation encoding, could condense the name to encodeRelativeParachain
.
No description provided.