-
Notifications
You must be signed in to change notification settings - Fork 360
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
bugfix: celo transfers content being passed to the device #8840
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
3 Skipped Deployments
|
@@ -118,6 +118,7 @@ const buildTransaction = async (account: CeloAccount, transaction: Transaction) | |||
|
|||
celoTransaction = { | |||
...celoTransaction, | |||
value: value.toFixed(), |
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.
broadcast fails when setting value here:
Transaction has been reverted by the EVM:
{
"blockHash": "0xe74a3082ed28fd566e7171335ec751b939b3e240bdff9a27a69f4ea677f21f42",
"blockNumber": 29731228,
"contractAddress": null,
"cumulativeGasUsed": 5769523,
"effectiveGasPrice": 10000000000,
"from": "0x849af8f177065ab056ed7bef4e473f5e7a6cee90",
"gasUsed": 27078,
"logs": [],
"logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
"status": false,
"to": "0x471ece3750da237f93b8e339c536989b8978a438",
"transactionHash": "0x7822bba8331a92a9b6584fd908bd06a6b401395b726661df4aa8c1f5ee0477f9",
"transactionIndex": 43,
"type": "0x0"
}
@@ -112,12 +112,14 @@ const buildTransaction = async (account: CeloAccount, transaction: Transaction) | |||
from: account.freshAddress, | |||
to: celoToken.address, | |||
data: celoToken.transfer(transaction.recipient, value.toFixed()).txo.encodeABI(), | |||
// value: `0x${value.toString(16)}`, // NOTE: this breaks at the estimateGas just below |
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.
setting value here (be it as value: value.toFixed
or as an hex string, fails at the gas estimation (const gas = await kit.connection.estimateGasWithInflationFactor(celoTransaction);
)
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.
it looks like its sending CELO via an erc20 transfer. (ie data is populated) but a simple native transfer is cheaper and probably all thats needed.
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.
Any idea why this had to be done? Commit message of the code i've replaced mentions Use erc20 for Celo token to fix precision issues
. Maybe this went around a legacy behaviour
A clue might lie in how celo cli craft their 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.
nice
β Checklist
npx changeset
was attached.π Description
Replace this text by a clear and concise description of what this pull request is about and why it is needed. Be sure to explain the problem you're addressing and the solution you're proposing.
For libraries, you can add a code sample of how to use it.
For bug fixes, you can explain the previous behaviour and how it was fixed.
In case of visual features, please attach screenshots or video recordings to demonstrate the changes.
Before (transfer to 0x1D5F344C7d5E7C3Fea434989B928fAb21B263246)
After
β Context
π§ Checklist for the PR Reviewers