-
Notifications
You must be signed in to change notification settings - Fork 7
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
[Unit test] role based key fee payer #59
base: dev
Are you sure you want to change the base?
[Unit test] role based key fee payer #59
Conversation
console.log("\n--- Generating Temporary Accounts ---"); | ||
roleTransactionAccount = generateTemporaryAccount(); | ||
roleAccountUpdate = generateTemporaryAccount(); | ||
roleFeePayerAccount = generateTemporaryAccount(); |
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.
[Feedback 1] fixed this test to generate temporary key and update the account key to role based.
cc. @kjeom
// Feedback2. Remove the dependencies with network connection. just check the signed result. | ||
// Feedback3. signed by a user (from address,roleTransactionAccount) first | ||
const signedTxBySender = await roleTransactionAccount.signTransaction(feeDelegatedTx); | ||
assert.isNotNull(signedTxBySender.rawTransaction, "Transaction should be signed by RoleTransaction"); |
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.
[Feedback 2] don't send the tx to the testnet, just check the signed result.
cc. @kjeom
// Feedback3. FeePayer(FeePayerAccount) sign the tx after the user's sign. | ||
try { | ||
const signedTxByFeePayer = await roleFeePayerAccount.signTransaction({ | ||
senderRawTransaction: signedTxBySender.rawTransaction |
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.
[Feedback 3] It should be signed by a user (from address) first. FeePayer normally sign the tx after the user's sign.
Actually, I have two questions about this Feedback3.
-
when you said "It should be signed by a user (from address) first. FeePayer normally sign the tx after the user's sign.", Is it correct to sign with roleTransactionAccount(user who signs the transaction account) first and then append roleFeePayerAccount to the rawTransaction of that signature?
-
If there is one unexpected issue in this test, it is “Returned error: insufficient funds for transfer”. I created temporary addresses via [Feedback 1], but now that accounts don't have enough klay(kaia). Is there a good way to charge klay(kaia) to the temporary accounts...? (I can't think of a good way... T^T...)
cc. @kjeom
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.
@minminkikiki "A user" means another account that is used by user, not feePayer.
roleTransactionKey and roleFeePayerKey are owned by feePayer. That means all keys of each role belong to another key owner who is not the user.
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.
@minminkikiki If you got the new error regarding the sign of feepayer, please add the description of the error in the PR description.
…d key, test case 3,4
// Test Case 3: Fee Delegated transaction signed by RoleFeePayer key (should succeed) | ||
it("3. Fee Delegated transaction signed by RoleTransaction and RoleFeePayer keys", async function () { | ||
// Feedback 4. Create a new user account (different user than FeePayer) | ||
const userAccount = generateTemporaryAccount(); |
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.
Hello @kjeom, I have reflected your feedback as follows.
[Feedback 4]
- As you mentioned, I have supplemented it so that a general user(userAccount) signs the transaction first, and then an additional signature is made with roleFeePayerAccount so that FeePayer can pay the fee on behalf of the general user.
[Feedback 5]
- minor, but changed the pubKey name you told.
[Feedback 6]
- Except for checking for errors, all other console.logs have been removed.
[New Error]
- I changed it as you advised, but I am facing a new error called "insufficient funds for transfer" which is different from the previous RLP error.
[Questions]
So, I have 2 questions about this.
-
Are TxType and Role-based key related to each other? (For Example, is it possible to sign Tx.Type.AccountUpdate only with roleAccountUpdatePubKey? or sign Tx.Type.FeeDelegatedValueTransferonly only with roleFeePayerPubkey?)
-
Is it true that the error "insufficient funds for transfer" is caused by the fact that the newly created account does not have kaia(fund)?
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.
- yes accountUpdate only can be signed by roleAccountUpdatePrivateKey, not Pubkey when the account is role-based. And if you want to pay the gasFee as a feePayer with role-based account, yes you should sign it with roleFeePayerPrivateKey.
- No, I think you should fill all kinds of field manually. This error occurred when calling some functions like estimateGas, getting current nonce. I think we need to add all fields manually to avoid calling the RPC functions to the node.
gasLimit: 100000, | ||
gasPrice: "25000000000", | ||
nonce: "0x0", | ||
chainId: "0x1001" |
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.
Thanks @kjeom , thanks to your advice, I solved the "insufficient funds" error.
[Feedback]
- Manually input transaction fields
- Changed to manually input gasPrice, gasLimit, nonce, chainId to solve the "insufficient funds for transfer" error.
- This prevents RPC calls such as eth_estimateGas, eth_getTransactionCount, etc., so that the problem does not occur even on test accounts without Kaia balance.
And this solved the "insufficient funds for transfer" error.
[Unexpected Observations]
But unfortunately? The unit test succeeded, so I was worried a lot. (Because if there is a bug, it should not succeed)
Test case 3: Expected bug does not appear
- I expected it to fail due to role mismatch when signing with Role-based FeePayer key in FeeDelegatedValueTransfer transaction, but it succeeded.
Test Case 4: Unexpected Success
- I expected it to fail when signing a FeeDelegatedValueTransfer transaction with roleTransactionAccount, but it succeeded.
[Result & Question]
I was wondering why it succeeded, and I thought that maybe it was a bug.
- This is because the signTransaction method in Kaia's web3js-ext does not perform role-based verification.
- Or role-based verification is only performed at the network level in the blockchain node.
- So it seems that the signTransaction method simply generates a signature and does not check the role restriction, so it is signed normally as above.
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.
@minminkikiki If you try RLP decoding below code, you can check the feePayer address is different address that is originally intended.
const { KlaytnTxFactory } = require("@kaiachain/web3js-ext");
const decoded = KlaytnTxFactory.fromRLP(signedTx.rawTransaction);
console.log(decoded)
// 4) RLP Decoding | ||
const { KlaytnTxFactory } = require("@kaiachain/web3js-ext"); | ||
const decoded = KlaytnTxFactory.fromRLP(signedTxByFeePayer.rawTransaction); | ||
console.log("Decoded Transaction:", decoded); |
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.
@kjeom
[What I Tried]
- Decoding (RLP) to Verify FeePayer Address
- I attempted to check whether the actual FeePayer address matches my intended address by using KlaytnTxFactory.fromRLP(...) after signing the transaction twice (User first, then roleFeePayerAccount).
- Once both signatures were complete, I decoded the final raw transaction, expecting to see the FeePayer field populated with my intended FeePayer address.
- Observation
- However, after decoding, I found that the feePayer field was empty, suggesting that the FeePayer signature wasn’t reflected in the final transaction.
[Question]
“If the feePayer field is empty in the decoded transaction, does that imply our two-step signing process is incorrect, or could there be a bug in how web3js-ext handles RoleBased FeePayer signatures?”
More specifically:
- I called signTransaction twice (User → FeePayer), providing all necessary fields (type, from, to, value, gasLimit, gasPrice, nonce, chainId, feePayer, and senderRawTransaction).
- Despite both signings appearing to succeed, the decoded result shows no FeePayer data.
[Additional Notes]
- This issue arises when using RoleBased accounts for FeeDelegated transactions. Even though the second signing step looks successful (i.e., I get a new raw transaction), the final RLP does not contain the feePayer field.
- I'm curious if this is related to a known RoleBased + FeeDelegated limitation or bug in web3js-ext...?
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.
@minminkikiki Could you send the transaction to the network? and check whether the actual feePayer's address is the role-based account's address.
In this case, you need to make the account like following
legacy A : address A - private Key A
legacy B : address B - private Key B
role based C : address A - transaction key A, fee payer key B
Test Overview
Reference
Understanding of the author, about Role-based Key
(If there was a misunderstanding, the test case itself could be wrong....)
(e.g RoleAccountUpdate(Role) : AccountUpdate(TxType)
RoleTransaction(Role) : ValueTransfer(TxType))
Test Cases
Test Results
Test Bug?