-
Notifications
You must be signed in to change notification settings - Fork 45
Tool 393 add relayer v 3 support #543
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
Conversation
src/proto/serializer.ts
Outdated
| if (transaction.relayer && !transaction.relayer.isEmpty()) { | ||
| protoTransaction.Relayer = transaction.relayer.getPublicKey(); | ||
| protoTransaction.RelayerSignature = transaction.relayerSignature; | ||
| } |
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.
Let's extract to a function similar to the guardian check above, and also check if relayer signature is set.
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 discussed about this internally and said it is not needed
src/transaction.spec.ts
Outdated
| it("should serialize transaction with relayer", async () => { | ||
| const transaction = new Transaction({ | ||
| chainID: networkConfig.ChainID, | ||
| sender: wallets.alice.address.bech32(), |
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.
use toBech32(), here and bellow.
src/transaction.ts
Outdated
| /** | ||
| * The relayer, in address format, next version all the other addresses will not be string anymore. | ||
| */ | ||
| public relayer?: Address; |
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.
maybe this doesn't need to be optional. Make it optional on the constructor and if not provided, set to Address.empty()?
src/transactionComputer.ts
Outdated
| applyRelayer(transaction: ITransaction, relayer: Address) { | ||
| transaction.relayer = relayer; | ||
| } |
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 is kind of useless since one can simply do:
transaction.relayer = relayerAddress;| chainID: transaction.chainID.valueOf(), | ||
| version: transaction.version, | ||
| options: transaction.options == 0 ? undefined : transaction.options, | ||
| relayer: !transaction.relayer || transaction.relayer.isEmpty() ? undefined : transaction.relayer.toBech32(), |
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 should have relayer and guardian done in the same way. Which way should we choose?
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.
Oh, now I see, cannot be same as guardian, since that's a string 👍
| nonce: BigInt(object.nonce), | ||
| value: BigInt(object.value || ""), | ||
| receiver: object.receiver, | ||
| relayer: object.relayer ? Address.newFromBech32(object.relayer) : Address.empty(), |
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.
Can we have it the same as guardian? If so, which way?
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.
Oh, now I see, cannot be same as guardian, since that's a string 👍
src/proto/serializer.ts
Outdated
| protoTransaction.GuardianSignature = transaction.guardianSignature; | ||
| } | ||
|
|
||
| if (transaction.relayer && !transaction.relayer.isEmpty()) { |
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.
Can refactor to have a mini private function isRelayedTransaction (for symmetry) .
src/transaction.ts
Outdated
| public guardian: string; | ||
|
|
||
| /** | ||
| * The relayer, in address format, next version all the other addresses will not be string anymore. |
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.
Can be rephrased.
The relayer address.
Note: in the next major version, `sender`, `receiver` and `guardian` will also have the type `Address`, instead of `string`.
src/transaction.ts
Outdated
| this.version = Number(options.version?.valueOf() || TRANSACTION_VERSION_DEFAULT); | ||
| this.options = Number(options.options?.valueOf() || TRANSACTION_OPTIONS_DEFAULT); | ||
| this.guardian = options.guardian ? this.addressAsBech32(options.guardian) : ""; | ||
| this.relayer = options.relayer?.isEmpty() ? undefined : options.relayer; |
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.
Oh, now I see, cannot be same as guardian, since that's a string 👍
src/transactionComputer.ts
Outdated
| transaction.guardian = guardian; | ||
| } | ||
|
|
||
| applyRelayer(transaction: ITransaction, relayer: Address) { |
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.
In sdk-py, we don't have it:
Should we have it?
https://github.com/multiversx/mx-sdk-specs/blob/main/core/transaction_computer.md
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 suggest we delete it from specs, as well. It is useless.
src/transactionComputer.ts
Outdated
| } | ||
|
|
||
| isRelayedV3Transaction(transaction: ITransaction) { | ||
| if (transaction.relayer && !transaction.relayer.isEmpty()) { |
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.
Can use a one-line return (can also stay as it is).
src/transaction.ts
Outdated
| /** | ||
| * The relayer, in address format, next version all the other addresses will not be string anymore. | ||
| */ | ||
| public relayer?: Address; |
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.
Indeed, optional - I see we have the same in PY 👍
https://github.com/multiversx/mx-sdk-py/blob/feat/next/multiversx_sdk/core/transaction.py#L49
|
What about the Was that an oversight or is it by design? |
|
Hello @MWFIAE, |
No description provided.