-
Notifications
You must be signed in to change notification settings - Fork 37
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
EN-13168: added relayed v1 tx builder #235
Conversation
# Conflicts: # package-lock.json
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.
Approve: I don't see any issue, only small beautifiers. I may do the changes when support for frozen account will be needed for relayed transactions as well.
src/relayedTransactionV1Builder.ts
Outdated
import BigNumber from "bignumber.js"; | ||
|
||
export class RelayedTransactionV1Builder { | ||
|
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.
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.
done
src/relayedTransactionV1Builder.ts
Outdated
} | ||
|
||
/** | ||
* Sets the network config to be used for building the relayed v2 transaction |
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.
* Sets the network config to be used for building the relayed v2 transaction | |
* Sets the network config to be used for building the relayed v1 transaction |
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.
fixed
src/relayedTransactionV1Builder.ts
Outdated
} | ||
|
||
/** | ||
* Sets the address of the relayer (the one that will actually pay the fee |
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.
* Sets the address of the relayer (the one that will actually pay the fee | |
* Sets the address of the relayer (the one that will actually pay the fee) |
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.
fixed
src/relayedTransactionV1Builder.ts
Outdated
.setFunction(new ContractFunction("relayedTx")) | ||
.setArgs([ | ||
new StringValue(serializedTransaction), | ||
//new StringValue(serializedTransaction.toString("hex")) |
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 don't see why this should stay here.
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.
true, removed
builder = builder | ||
.setNetworkConfig(networkConfig) | ||
.setInnerTransactionGasLimit(10) | ||
.setInnerTransaction(innerTx) | ||
.setRelayerAddress(alice.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.
👍
@@ -68,6 +76,7 @@ describe("test relayed v2 transaction builder", function () { | |||
.setInnerTransaction(innerTx) | |||
.setInnerTransactionGasLimit(60_000_000) | |||
.setNetworkConfig(networkConfig) | |||
.setRelayerAddress(alice.getAddress()) |
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.
👍
src/relayedTransactionV2Builder.ts
Outdated
this.netConfig = netConfig; | ||
return this; | ||
} | ||
|
||
/** | ||
* Tries to build the relayed v2 transaction based on the previously set fields | ||
* Sets the address of the relayer (the one that will actually pay the fee |
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.
Like the one before
* Sets the address of the relayer (the one that will actually pay the fee | |
* Sets the address of the relayer (the one that will actually pay the fee) |
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.
fixed
@@ -0,0 +1,81 @@ | |||
import {loadTestWallets, TestWallet} from "./testutils"; |
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.
Imports formatting is inconsistent with other files (optional).
nonce: 15, | ||
sender: alice.address, | ||
receiver: Address.fromBech32("erd1qqqqqqqqqqqqqqqpqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqzllls8a5w6u"), | ||
gasLimit: 10, |
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 more real-world value her (optional).
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.
done
const relayedTxV1 = builder | ||
.setInnerTransaction(innerTx) | ||
.setRelayerNonce({ | ||
valueOf: function () { |
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 think passing the number directly should work (since Number implements valueOf
).
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.
done
netConfig: INetworkConfig | undefined; | ||
|
||
/** | ||
* Sets the inner transaction to be used. It has to be already signed. |
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.
👍
* | ||
* @param {Transaction} transaction The inner transaction to be used | ||
*/ | ||
setInnerTransaction(transaction: Transaction): RelayedTransactionV1Builder { |
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.
Could we have received an ITransaction
here? Please leave as it is otherwise :)
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.
no, we cannot use that
const payload = TransactionPayload.contractCall() | ||
.setFunction(new ContractFunction("relayedTx")) | ||
.setArgs([ | ||
new StringValue(serializedTransaction), |
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.
👍
src/relayedTransactionV1Builder.ts
Outdated
receiver: this.innerTransaction.getSender(), | ||
value: 0, | ||
gasLimit: | ||
this.netConfig.MinGasLimit + this.netConfig.GasPerDataByte * payload.length() + this.innerTransaction.getGasLimit().valueOf(), |
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.
Optionally, this could have been a separate variable.
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.
done
|
||
const txObject = { | ||
"nonce": this.innerTransaction.getNonce().valueOf(), | ||
"sender": new Address(this.innerTransaction.getSender().bech32()).pubkey().toString("base64"), |
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, I've forgot about this 👍
@@ -67,10 +75,17 @@ describe("test relayed v2 transaction builder", function () { | |||
const relayedTxV2 = builder | |||
.setInnerTransaction(innerTx) | |||
.setInnerTransactionGasLimit(60_000_000) | |||
.setRelayerNonce({ |
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.
Could have been setRelayerNonce(37)
(I think).
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.
done
innerTransaction: Transaction | undefined; | ||
innerTransactionGasLimit: IGasLimit | undefined; | ||
relayerAddress: IAddress | undefined; |
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.
All right, they don't seem to be breaking changes.
b6e7c99
8efa383
Added a buidler for relayed v1 tx builder + some fixes for relayed v2