-
Notifications
You must be signed in to change notification settings - Fork 83
Transaction API #75
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
Transaction API #75
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.
excited for this!
return this; | ||
} | ||
|
||
public Transaction SetGas(string gas) |
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 this gas limit or gas price? should be explicit
var gasPrice = await GetGasPrice(); | ||
var gasCost = gasLimit * gasPrice; | ||
|
||
return new GasCosts { ether = gasPrice.ToString().ToEth(18, false), wei = gasCost }; |
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.
niiice
return new GasCosts { ether = gasPrice.ToString().ToEth(18, false), wei = gasCost }; | ||
} | ||
|
||
public async Task<Transaction> EstimateAndSetGasAsync() |
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.
when do I use this?
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.
If you manually change something like data/args or to generally do another estimation step. Might introduce a min limit default 100k in this function too? I'll also use it for my typed transactions later.
return this; | ||
} | ||
|
||
public async Task<string> Send(bool? gasless = null) |
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.
very nice
req.uploadHandler = (UploadHandler)new UploadHandlerRaw(bodyRaw); | ||
req.downloadHandler = (DownloadHandler)new DownloadHandlerBuffer(); | ||
req.SetRequestHeader("Content-Type", "application/json"); | ||
await req.SendWebRequest(); |
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.
the gasless code is re-used with the transaction manager no? might be useful to consolidate
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.
Will likely be moving the execution steps from TransactionManager to Transaction later, also contains some differences atm.
@joaquim-verges would appreciate another review while I do the WebGL side <3 |
Transactoooooooooooooor