Implement fee estimate #325

Nutomic · 2023-01-11T14:04:23Z

Very much work in progress

Summary of changes

Add API endpoint to calculate fees
Estimate transaction fee using etherscan.io API
Calculate token exchange rate using coingecko.com

Todo

Reference issue to close (if applicable)

Closes

Code Checklist

Tested
Documented

Nutomic · 2023-01-12T12:51:29Z

Updated. The basic functionality is working according to the spec as far as I can tell. However there are a lot of todos where things are unclear, please have a look.

The main task now is to retrieve the actual wrappedToken and baseToken from the transaction. I looked through the code but couldnt find anything related. Any hints how to do this?

crates/tx-relay-utils/src/lib.rs

dutterbutter · 2023-01-12T14:40:41Z

Updated. The basic functionality is working according to the spec as far as I can tell. However there are a lot of todos where things are unclear, please have a look.

The main task now is to retrieve the actual wrappedToken and baseToken from the transaction. I looked through the code but couldnt find anything related. Any hints how to do this?

@shekohex @drewstone any suggestions?

drewstone

Good start, hopefully my answers help give more direction here. Let me know if some things aren't clear.

crates/relayer-handlers/src/routes/fees.rs

drewstone · 2023-01-13T05:31:15Z

crates/relayer-handlers/src/routes/fees.rs

+    // TODO: hardcoded
+    let estimated_gas_amount = U256::from(1_721_713);
+
+    // TODO: need to get the actual tokens which are being exchanged (also in handle_vanchor_relay_tx)


Correct, currently each "bridge" or "set of anchors" is directly coupled with a specific Webb wrapped token. If a relayer receives a request to submit a transaction against Anchor A on Ethereum then we should be able to directly check A.token().

You can organize as you think is best, perhaps with function args or new structs.

crates/relayer-utils/src/fees.rs

drewstone · 2023-01-13T05:33:14Z

crates/relayer-utils/src/fees.rs

+    wrapped_token: &str,
+    base_token: &str,
+) -> f64 {
+    let tokens = &[wrapped_token, base_token];


At launch the wrapped_token is likely not going to be listed on any exchanges or markets and therefore no prices. The division at the end seems problematic. Let's just use the base_token here.

drewstone · 2023-01-13T05:38:53Z

crates/relayer-utils/src/fees.rs

+
+/// Estimate gas price using etherscan.io. Note that this functionality is only available
+/// on mainnet.
+async fn estimate_gas_price() -> crate::Result<u64> {


To your previous question about support for different blockchains, here is a perfect function (as many of the others also are) to parameterize this by the chain. Currently, we have the notion of a TypedChainId that we use to identify chains and ecosystems. You can find this in webb-rs and in Typescript.

The idea with our TypedChainId is that it identifies a blockchain ecosystem and a chain in that ecosystem. We needed a unique identifier that could work for different ecosystems where chains shared the same chain identifier. It fits into a u48 (or 6-bytes), but we use u64 more commonly and reserve 8-bytes for this identifier in our protocol messages. An example of an 8-byte (or 6-byte.. drop the 0x0000) TypedChainId is 0x0000010000000001. The last 4-bytes represent the u32 valued chain identifier of all EVM and Substrate based chains.

We could match on different ecosystems/chain identifiers and switch where and how we query for gas. The only two relevant ecosystems atm are EVMs and Substrate chains.

For future consideration, we have in the past tried to integrate this protocol with CosmWasm (Cosmos based smart contract system) chains. In Cosmos, people use very long chain identifiers. So while we have this u64 based identifier which supports nearly every ecosystem, it does not support Cosmos chains.

Example: https://github.com/cosmos/chain-registry/blob/master/gravitybridge/chain.json#L8 (I think this exceeds a u64)

I suppose as we start to consider v2 of the protocol we really do need to ensure our protocol messages support the variation in chain identifiers, addresses, etc.

In Rust the normal solution for such different data types is something like:

enum ChainTypes { Ethereum(u64), Cosmos(String), }

Serde has a couple of different ways to serialize this

Edit: So it this would be a change to TypedChainId.

Remember we need a binary format that so small and so efficient, easy to implement and test. Not just that, but also it should be fairly easy to implement in different languages and platforms.

In that case we could take a hash of the chain id string, and store the first 64 bits of that hash. That would be enough to check that the chain id matches another id.

We could do hashing yes, in that case it might fit into our currently allotted 6 bytes. This is actually what we had already planned for the cosmos work.

Nonetheless, the main focus initially is support for EVM and Substrate chains (where we have our current implementations). I would also please ensure you try and leverage the tools we already have built rather than create new ones, specifically the TypedChainIds for identifying chains.

I mentioned that in my other pull request:

I tried to use TypedChainId but thats not possible because Typescript tests use randomly generated chain ids, and converting them to TypedChainId fails. At the same time I noticed that there are many different types used for chain id (string, u32, u64). It would be good to refactor this in the future for more consistency.

crates/relayer-utils/src/fees.rs

crates/tx-relay/src/evm/vanchor.rs

drewstone · 2023-01-13T05:53:13Z

crates/relayer-handlers/src/lib.rs

+/// Handler for fee estimation
+///
+/// TODO: do we need to add another endpoint for substrate?
+pub async fn handle_fee_info() -> Result<impl warp::Reply, Infallible> {


Coming back to this now at the end of my review after understanding the flows more..

I think we need to parameterize these functions or wrap them in some parameterizable struct by the

TypedChainId

Transaction type (2 input transfer, 16 input transfer, 2 input withdrawal, 16 input withdrawal)

VAnchor address in question, after all the honest user is requesting info since they're about to relay a transaction spending some funds whose destination chain is the TypedChainId.

This will tell us the type of chain under consideration so we can switch APIs and gas estimation issues.

drewstone · 2023-01-13T05:56:19Z

For dealing with the Webb wrapped token in case I didn't add enough details, I would recommend checking out the API for submitting a relayable transaction. In that logic, there should be a clear VAnchor contract address that is being called. On that contract there's a getter function token(): address. If we can grab an instance of the VAnchor contract then we can get its token and from there its base tokens, etc.

Nutomic · 2023-01-13T11:34:28Z

Im currently working through the comments and added the caching of FeeInfo (single instance, valid for one minute).

I also added fee info to the test case should relay private transaction. After adding the fee to signatureVBridge.transact() call, it throws an error UNPREDICTABLE_GAS_LIMIT. The suggested solution is to manually set a gas limit, but transact() has no such method. So its probably necessary to add this param in webb-tools library, or maybe there is another solution.

- remove static var - add comments - add fee logic for handle_vanchor_relay_tx

- better number types - fixed refund of 1 usd which gets converted to wrapped token - validate refund amount - remove params for fee_info endpoint

…before)

- Automatically compile relayer when tests are executed - Add refundWallet for test

Nutomic · 2023-01-19T06:35:52Z

Salman suggested to merge this PR first and finish the remaining tasks in a separate PR, so that its easier to review. We can do this if you want, but the code is still not fully working, except in a very specific scenario (certainly not ready for deployment). I will list the main changes and remaining tasks here:

Main changes:

Added fee_info API endpoint /api/v1/fee_info:

{
    "estimatedFee": "0x476b26e0f",
    "gasPrice": "0x11",
    "refundExchangeRate": "0x28f",
    "maxRefund": "0xf3e59",
    "timestamp": "2023-01-19T06:29:49.556114073Z"
}

Removed WithdrawConfig which is now unused
Updated test should relay private transaction to use fee info
Tests now use cargo run instead of directly executing binary in target folder, so it is automatically recompiled if anything changed in the Rust code

Todo now:

Relaying currently tends to fail fee check, because it estimates gas amount in a different way which usually higher than the hardcoded gas amount used in fee_info API endpoint. Need to decide how to handle this.
There is no error handling, everything is unwrapped
Fix tests

Todo potentially in separate PRs:

Token names are still hardcoded to USDC and Ethereum, need to get actual tokens
Checking the refund amount in test "should relay private transaction" is currently not working (Salman said he would debug this)
fee info needs param for transaction type (number of inputs/outputs), and use different gas amount estimate accordingly
fee info is cached globally, should be separate cache entries per user/token/chain
Relayer should make a profit, but this is currently not the case. Probably need to increase exchange rate by 1% or something similar.
Need to add fee estimation endpoint for substrate

Nutomic · 2023-01-24T01:31:18Z

This is ready to review. After review is done, I we can keep the PR open and wait until I finish the remaining tasks in a second PR, then merge both of them together. This way we avoid breaking the main branch, as the code currently only handles fees for relaying USDC on Ethereum.

shekohex

LGTM! Good job @Nutomic

Looking forward for the new PRs!

drewstone · 2023-01-31T01:06:19Z

This looks great too! Great work @Nutomic

drewstone · 2023-01-31T02:35:27Z

We aren’t actually dealing with any string chain ids at the moment too. Cosmos was just mentioned for the future consideration. We can hash to u32 as well in that case.

…

On Mon, Jan 30, 2023 at 7:34 PM Nutomic ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In crates/relayer-utils/src/fees.rs <#325 (comment)>: > + wrapped_token: &str, + base_token: &str, +) -> f64 { + let tokens = &[wrapped_token, base_token]; + let prices = COIN_GECKO_CLIENT + .price(tokens, &["usd"], false, false, false, false) + .await + .unwrap(); + let wrapped_price = prices[wrapped_token].usd.unwrap(); + let base_price = prices[base_token].usd.unwrap(); + wrapped_price / base_price +} + +/// Estimate gas price using etherscan.io. Note that this functionality is only available +/// on mainnet. +async fn estimate_gas_price() -> crate::Result<u64> { I mentioned that in my other pull request: I tried to use TypedChainId but thats not possible because Typescript tests use randomly generated chain ids, and converting them to TypedChainId fails. At the same time I noticed that there are many different types used for chain id (string, u32, u64). It would be good to refactor this in the future for more consistency. — Reply to this email directly, view it on GitHub <#325 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADELLF77V7LWQKBRSSX5GITWVB2Z7ANCNFSM6AAAAAATYCP2RE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

drewstone · 2023-01-31T02:36:08Z

Can you say more about the errors using the TyoedChainId in Typescript? I’m pretty sure we have examples using it.

…

On Mon, Jan 30, 2023 at 7:35 PM Drew Stone ***@***.***> wrote: We aren’t actually dealing with any string chain ids at the moment too. Cosmos was just mentioned for the future consideration. We can hash to u32 as well in that case. On Mon, Jan 30, 2023 at 7:34 PM Nutomic ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In crates/relayer-utils/src/fees.rs > <#325 (comment)>: > > > + wrapped_token: &str, > + base_token: &str, > +) -> f64 { > + let tokens = &[wrapped_token, base_token]; > + let prices = COIN_GECKO_CLIENT > + .price(tokens, &["usd"], false, false, false, false) > + .await > + .unwrap(); > + let wrapped_price = prices[wrapped_token].usd.unwrap(); > + let base_price = prices[base_token].usd.unwrap(); > + wrapped_price / base_price > +} > + > +/// Estimate gas price using etherscan.io. Note that this functionality is only available > +/// on mainnet. > +async fn estimate_gas_price() -> crate::Result<u64> { > > I mentioned that in my other pull request: > > I tried to use TypedChainId but thats not possible because Typescript > tests use randomly generated chain ids, and converting them to TypedChainId > fails. At the same time I noticed that there are many different types used > for chain id (string, u32, u64). It would be good to refactor this in the > future for more consistency. > > — > Reply to this email directly, view it on GitHub > <#325 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ADELLF77V7LWQKBRSSX5GITWVB2Z7ANCNFSM6AAAAAATYCP2RE> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

Nutomic · 2023-01-31T03:55:13Z

@drewstone TypedChainId doesnt support the chain id which is used in Typescript tests which is 3334. Calling TypedChainId::from(3334) returns None. So passing the id this way doesnt work. A possible solution would be to allow arbitrary chain ids only in debug mode.

drewstone · 2023-01-31T03:58:28Z

@Nutomic please refer to: https://github.com/webb-tools/webb-rs/blob/main/proposals/src/header.rs.

There is a method to get it from a u64. But really I want you to read how we construct it from bytes as that is the encoding that we want. As far as you've mentioned, 3334 fits within a u32. If it's an EVM chain the prefixed 2 bytes should reflect that.

Nutomic · 2023-01-31T12:31:43Z

Closing this one, we can merge #330 only.

shekohex marked this pull request as draft January 11, 2023 14:13

Nutomic commented Jan 12, 2023

View reviewed changes

crates/tx-relay-utils/src/lib.rs Show resolved Hide resolved

Nutomic force-pushed the fee-estimate branch 4 times, most recently from 88637a6 to eeb8f87 Compare January 12, 2023 14:10

salman01zp added the wip 🚧 Work in-progress label Jan 12, 2023

drewstone requested changes Jan 13, 2023

View reviewed changes

Nutomic force-pushed the fee-estimate branch from d2ca02e to a8c4dc5 Compare January 13, 2023 08:38

Felix Ableitner added 14 commits January 19, 2023 15:33

WIP: Implement fee estimate

f726003

Various changes

60c1281

- remove static var - add comments - add fee logic for handle_vanchor_relay_tx

More changes

3718bf5

- better number types - fixed refund of 1 usd which gets converted to wrapped token - validate refund amount - remove params for fee_info endpoint

Update comments, add error for invalid refund value

8626685

Store clients in static variables

177932a

Use etherscan API key from env

2960749

Remove unused WithdrawalConfig (withdraw_gaslimit was already unused …

1612f8c

…before)

Add caching for FeeInfo, write test code (currently failing)

2476422

Use BigNumber type in tests

f1fed27

Improve tests

d95e670

- Automatically compile relayer when tests are executed - Add refundWallet for test

Convert exchange rate to U256

7556bb2

Tests: add fee/refund to withdrawal transaction

88e8752

Test working

54b1785

Various changes

44608e9

Nutomic force-pushed the fee-estimate branch from 59fafa7 to 44608e9 Compare January 19, 2023 06:33

Nutomic requested a review from salman01zp January 20, 2023 01:50

Error handling

ec7aa1e

Nutomic force-pushed the fee-estimate branch from 2a40a9c to ec7aa1e Compare January 20, 2023 02:53

Felix Ableitner and others added 4 commits January 20, 2023 12:23

Fix fee calculation and allow for some variation

687e661

Merge branch 'main' into fee-estimate

48ba78c

Merge error enums

ef06c99

Use etherscan api key in ci

086f1ce

Nutomic marked this pull request as ready for review January 24, 2023 01:28

Nutomic changed the title ~~WIP: Implement fee estimate~~ Implement fee estimate Jan 24, 2023

Nutomic mentioned this pull request Jan 24, 2023

Fee estimate part 2 #330

Merged

2 tasks

shekohex approved these changes Jan 24, 2023

View reviewed changes

Nutomic closed this Jan 31, 2023

shekohex deleted the fee-estimate branch March 8, 2023 18:10

Nutomic mentioned this pull request May 17, 2023

[TASK] Remove the need for TypedChainIds in the API Endpoints #510

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement fee estimate #325

Implement fee estimate #325

Nutomic commented Jan 11, 2023 •

edited

Loading

Nutomic commented Jan 12, 2023

dutterbutter commented Jan 12, 2023

drewstone left a comment

drewstone Jan 13, 2023

drewstone Jan 13, 2023

drewstone Jan 13, 2023

drewstone Jan 13, 2023

drewstone Jan 13, 2023

Nutomic Jan 13, 2023 •

edited

Loading

shekohex Jan 13, 2023

Nutomic Jan 16, 2023

drewstone Jan 31, 2023

Nutomic Jan 31, 2023

drewstone Jan 13, 2023

drewstone commented Jan 13, 2023

Nutomic commented Jan 13, 2023 •

edited

Loading

Nutomic commented Jan 19, 2023 •

edited

Loading

Nutomic commented Jan 24, 2023

shekohex left a comment

drewstone commented Jan 31, 2023

drewstone commented Jan 31, 2023 via email

drewstone commented Jan 31, 2023 via email

Nutomic commented Jan 31, 2023 •

edited

Loading

drewstone commented Jan 31, 2023

Nutomic commented Jan 31, 2023

Implement fee estimate #325

Implement fee estimate #325

Conversation

Nutomic commented Jan 11, 2023 • edited Loading

Summary of changes

Todo

Reference issue to close (if applicable)

Code Checklist

Nutomic commented Jan 12, 2023

dutterbutter commented Jan 12, 2023

drewstone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewstone commented Jan 13, 2023

Nutomic commented Jan 13, 2023 • edited Loading

Nutomic commented Jan 19, 2023 • edited Loading

Nutomic commented Jan 24, 2023

shekohex left a comment

Choose a reason for hiding this comment

drewstone commented Jan 31, 2023

drewstone commented Jan 31, 2023 via email

drewstone commented Jan 31, 2023 via email

Nutomic commented Jan 31, 2023 • edited Loading

drewstone commented Jan 31, 2023

Nutomic commented Jan 31, 2023

Nutomic commented Jan 11, 2023 •

edited

Loading

Nutomic Jan 13, 2023 •

edited

Loading

Nutomic commented Jan 13, 2023 •

edited

Loading

Nutomic commented Jan 19, 2023 •

edited

Loading

Nutomic commented Jan 31, 2023 •

edited

Loading