api3dao / airnode Goto Github PK

One can create a template with a very large parameters field, which would cause the Convenience method to revert due to the exceeding the gas limit, resulting in not being able to retrieve other 9 templates. I'll investigate further.

• convenience.getTemplates (#184)
• convenience.getAuthorizationStatuses

Template format used to be

Now it is

getTemplates() from Convenience changed accordingly

Types and contract calls need to be updated.

This tends to be a lower than "fast" gas price, but it may be a better idea than having a constant minimum value of 40 gwei

https://docs.ethers.io/v4/api-providers.html?highlight=gasprice#blockchain-status

Using .solhintignore to ignore node_modules doesn't work

https://github.com/clc-group/airnode/blob/f7fd762dba2e1bd79459c140e1edbc3f827d10e9/package.json#L7

No need to validate that walletInd is not 0 (we don't have walletInd anymore in the first place) or that its balance is more than minBalance.

All code related to walllet checks need to be removed.

It should return early

Description

API calls may or may not be idempotent so the API provider might not want to make duplicate API calls between serverless function invocations while the transaction is pending. The API response needs to be optionally cached.

The cache should store the following information against the requestId:

1. The raw value extracted from _path or the error
2. The timestamp
3. The status boolean - either successful or failure.

Cache keys need to have an expiration mechanism too.

Before making an API call, the cache should be checked for the requestId. If a value is found, skip making that call and attempt another transaction with that value.

API calls should default to cached if a cache is configured. Otherwise, a new call should be made each serverless invocation.

Questions

1. How exactly to implement the cache. What AWS service to use etc.
2. When should the cache key be cleared? If a requestid exists in the cache, but no unfulfilled request is found?

There used to be a method to error a call

All code about erroring needs to be removed. The node should try to call fulfill() and if that reverts, it should call fail() directly.

Request IDs are derived from request parameters. The derivation changes with type:

Regular:

When the node receives a request, it should verify this derivation to ensure that the Ethereum provider didn't tamper with any of the request parameters.

Description

A user should be able to configure Ethereum providers. This will allow the node to be more resilient if a single provider is down and will also better protection if a node starts reporting bad (or even malicious) data. e.g. The current block number is 99999999.

Step 1

For each configured Ethereum provider, we need to find all of the relevant details at the start of the serverless invocation. This should be done in parallel. We'll need the following information for each provider:

1. The gas price
2. Current block number
3. The configured network
4. Pending requests

And possibly a few other things. This information should be store against each provider in the state.

Step 2

For unique request, the node should make a single API call. Each of these calls needs to happen in separate serverless functions. The response value should also be extracted in the separate function invocation using the _path parameter. API responses can be very large and we could easily hit the memory limit if multiple large responses get returned to the main function.

Step 3

For each configured Ethereum provider and each response, we make transaction. This means duplicate transactions. I don't think this step needs to be in separate serverless functions, but that is a possibility.

Having a gas price data feed on Ropsten (https://github.com/clc-group/airnode/blob/master/packages/node/src/core/ethereum/contracts/gas-price-feed.ts#L6) doesn't make sense because apparently miners don't care about gas prices while mining transactions. Similarly, people on permissioned chains etc. won't be running gas price feeds. Therefore, we can simplify how that works at the node.

The easiest solution I can think of is that the node assumes it's on mainnet, calls the data feed to get the gas price and only use it if it makes sense. This is safe because:
1 - You can't try to deploy a contract on Ropsten to have the exact same address as the mainnet gas price feed address
2 - Even if you did, it's Ropsten
I'll do some research on how this would work and report back.

The node used to get requester data from Convenience:
https://github.com/api3dao/airnode/blob/566fc7b72e1b22486cdc6e87d1e86c27d93ecacd/packages/node/src/core/evm/requests/requester-data.ts
It got requesterId, walletAddress, walletInd, walletBalance, minBalance.

Now, all requests include requesterInd and designatedWallet, there is no separate walletInd, and the minimum balance check is done at an Authorizer.

All code related to fetching requester data should be removed.

Authorization checks used to be for endpoint-client pairs:

Now they are request specific and have a bunch of other arguments (providerId is not an array!):

Also note that the non-batch version of this method is also in Convenience (will be used as fallback if the batch version reverts):

We want to provide a set of generic Authorizer contracts for the providers so that they can combine them to implement custom authorization policies.

It should be a two-step procedure where the new owner accepts the transfer. See
https://github.com/smartcontractkit/chainlink/blob/46d4a77faae2904b6b15b747edd5ba2955337034/evm-contracts/src/v0.6/dev/FluxAggregator.sol#L544

The contracts should derive request IDs (both API and withdrawal) from request-time parameters. For example the short request ID is derived as

requestId = keccak256(abi.encodePacked(
  noRequests,
  templateId,
  parameters
  ));

and emits

emit ShortRequestMade(
  providerId,
  requestId,
  msg.sender,
  noRequests
  templateId,
  parameters
  );

Then, the node can check if templateId or parameters is tampered with by the Ethereum provider by checking if their hash matches the requestId.

The node should do the same thing with templates, i.e., when it fetches a template, it should calculate its hash and check if it matches the template ID.

This should also be done for withdrawal requests

Leaving this here as a to do
#5 (comment)

If a requester reserves makes a transaction to reserve a wallet and the node does not see that event, the requester loses the authorizationDeposit and also gets locked out of being able to have a wallet reserved. The requester should be able to rebroadcast the reservation event without making any deposit for the node to be able to see and handle it.

Note that the node shouldn't serve duplicate events separately.

The authorization check shouldn't call the contract if authorizers is [] (return false automatically) or [ethers.constants.AddressZero] (return true automatically)

When idempotence is important, a client request should only be attempted to be fulfilled when the node is sure that the request exists, which requires a configurable parameter for how many blocks it should wait for. I think it's perfectly fine to not have this for now.

Tracked by https://api3dao.atlassian.net/browse/AN-160

More contracts using the feed = Less secure
We only want the contracts that have paid for the feed to be able to use it. See:
https://github.com/smartcontractkit/chainlink/blob/46d4a77faae2904b6b15b747edd5ba2955337034/evm-contracts/src/v0.6/dev/AccessControlledAggregator.sol#L40

ClientRequestCreated sounds good. The other names would go like ClientShortRequestCreated etc.?

and how about ClientRequestFulfilled / ClientRequestErrored / ClientRequestFailed ?

https://chainlinkconsulting.slack.com/archives/DD320C6P5/p1596449434002000?thread_ts=1596448826.001000&cid=DD320C6P5

Currently, withdrawal requests are prioritized and cause other requests made to the respective designated wallet to be ignored.

A malicious provider can't make an Airnode make a withdrawal by faking an event because the fulfillment transaction checks if such a withdrawal request exists on-chain
https://github.com/clc-group/airnode/blob/c1250cbc94f7c3e7c36ffa05edacede0d10eb699/packages/protocol/contracts/ProviderStore.sol#L344
so even if the node attempts to fulfill the faked withdrawal request, the fulfillment will revert.

However, with this scheme, the fake withdrawal event can still be used to put a specific designated wallet of an Airnode out of operation for 1 hour because it will ignore all other requests.

Therefore, withdrawal requests should not cause other requests to be ignored, and should be sent to the back of the nonce-queue.

and not sure if it's worth it

Request IDs are derived from request parameters, for example:

Here, we only need to test for requestId:

The node should attempt to connect to multiple providers on function run in case one or more are down. It should then choose between responding nodes and stick with that provider for the duration of the function.

Maybe it should check for the latest block among responding nodes to determine which provider to use. Not sure what should happen if multiple respond with the same block. Maybe the user should be allowed to rank them in terms of preference?

i.e.

ETHEREUM_PROVIDER_URL=xxx
ETHEREUM_BACKUP_PROVIDER_URL=xxx
ETHEREUM_BACKUP_PROVIDER_2_URL=xxx

(There are probably better names we could use)

Request fulfillment events used to be

The ABI needs to be updated

Right now, the client contracts announce their (potential) endorser under the public variable requesterId. Wallet addresses can't do the same, so they can't be endorsed. If these are kept it RequesterStore, wallet addresses can also be endorsed and make requests that can be fulfilled with a reserved wallet.

The normal mode of operation doesn't give any guarantees for not calling the API more than once for a single request:
https://api3dao.github.io/api3-docs/pre-alpha/airnode/implementation.html#non-idempotent-operations

The normal mode of operation also doesn't give any guarantees about being absolutely sure that an incoming request is real (and not spoofed for example).

We need to have a mode and guidelines for when the user wants to use non-idempotent operations, and thus only want to call the API when there is an actual request, and make that call only once.

• The user must only use trusted Ethereum providers. Otherwise, the Ethereum provider can send the user a spoofed request event and have them call the API. The fulfillment of this request will revert, but the main aim of the attack would be to have the user make a call to the API. Managed Ethereum node services largely resolve this issue.
• The node must wait for enough block confirmations (#41)
• The node most be configured (by setting a flag for example) to cache API calls (#26). When the node encounters a request, it should first check the cache to see if the call has already been made. If not, it should make the call and create a record at the cache.

Currently, we require a transaction to be made for an endpoint to be created on-chain, essentially to only create an endpoint ID. Instead, we can use a providerId-endpointInd pair to refer to endpoints. This would also be preferable as we would have identical endpointInds across chains (which can be thought together with mirroring the providerId across chains, see #55 ).

Request events used to be

Template ID is derived from its contents:

When the node receives a template, it should verify this to ensure that the Ethereum provider didn't tamper with any of the template fields.

Instead of getting the block number while initalizing the providers, the node should call Convenience to get the block number + provider data:

Note that this method is payable. This is because the provider will have to send some ETH to the wallet with path m for the node to be able to do this. While doing this, the node will send the remaining funds to admin.

Implement something like the below RetryProvider. I'm not sure that it's exported anywhere in ethers

https://github.com/ethers-io/ethers.js/blob/master/packages/experimental/src.ts/retry-provider.ts

This script seems to have been removed

https://github.com/clc-group/airnode/blob/d5ceae3936da859eff69f80dc5d7e13412e6e1a0/packages/contracts/package.json#L5
prestart doesn't refresh the ganache-cli instance if one already exists, but keeps the old one around. It would be ideal to refresh it to avoid persisting effects between runs.

Just use the requesterId as the walletInd

The tests commented out here spend inconsistent amounts of gas
f0ac82c#diff-55b6470a81b9afa68182a49553eedb12

Resetting ganache as below doesn't help

beforeEach(async () => {
  jest.resetModules();
  const ganache = require('ganache-core');
  ...

These tests are not important in particular, but ganache not resetting properly between tests is concerning (assuming the inconsistency is caused by the tests running in changing order).

There are two withdrawals:

• Made upon a withdrawal request
  

  airnode/packages/protocol/contracts/ProviderStore.sol

  Line 120 in 696ebad

  function fulfillWithdrawal(

• Made after a provider record is created
  

  airnode/packages/protocol/contracts/ProviderStore.sol

  Line 31 in 696ebad

  function createProvider(

In both cases, we need the node to send balance - txCost to the destination. Here, we should estimate the gas cost to estimate the transaction cost

This (i.e. not hardcoding the gas cost) good for three reasons:

1. Future forks may change instruction gas costs
2. The destination may be a payable contract instead of a regular wallet, in which case the gas cost would be variable
3. Gas cost may vary with the chain

and withdrawals won't be common, so the additional Ethereum call is not significant.

Assuming the provider will use a single private key for all chains, we can derive the providerId from that so that they would have identical providerIds in all chains.

• API calls are currently set to Errored (wrong)
• Withdrawals do not have their balance validated

During tests, ganache-core is giving this error message with no apparent problem

(node:1467) V8: /home/burak/git/airnode/packages/contracts/node_modules/ganache-core/node_modules/rustbn.js/lib/index.asm.js:2 Linking failure in asm.js: Unexpected stdlib member

A requester no longer needs to make a request to have a wallet designated and the node doesn't need to fulfill these.

All code related to wallet designations should be removed.

Fulfillment and failure arguments used to be

This is done for the fulfillment parameters to be checked on-chain. In the previous state, the provider was able to fulfill an existing request using providerId, designatedWallet, fulfillAddress and fulfillFunctionId that are different than the ones defined by the requester.

Calls to these methods need to be updated.

I realized that I made a mistake with the OIS. The security field should be a SecurityRequirement, not a SecurityRequirement[]. See:
https://github.com/clc-group/airnode/wiki/OIS-v1.0.0#43-security

The node should not be able to use any fulfillment parameter (fulfillAddress etc.) they want for a specific request. Their hash should be recorded at request time and checked for during fulfillment.

This would require both fulfill and error to be called with fullfillment and error parameters.

The initial plan was to use getGasPrice() and also get the gas price from a feed, then use the maximum. The assumption was that getGasPrice() may (will) be below what we want to use, so we can use the gas price feed to ramp up the gas prices temporarily.

This is both not a very elegant solution, and with the increased focus on serving non-mainnet chains, we should move towards a more chain-agnostic solution. My proposed solution is to only depend on getGasPrice(), but use a dead reckoning approach to bump gas prices as needed.

The main idea is to use k * getGasPrice() as the gas price, where k > 1 and increases with currentBlock - requestBlock. The exact coefficient (for example, what is k if the request is 20 blocks old and still not fulfilled?) will be determined based on off-chain statistical analysis of gas prices. This coefficient will be a lot more static than the gas price itself (so it doesn't matter if the gas price is 50 gwei or 500 gwei, k would be 1.5 if the request is 20 blocks old). Then we can hardcode this into config.json. There's also the possibility of keeping this coefficient on-chain (update every week for example), possibly by the API3 DAO or one of its teams.

All requests pass designatedWallet and requesterInd.

When the node receives a request, it should verify that designatedWallet is the address of m/0/${requesterInd} and ignore the request if this is not the case.