Drop unencrypted transactions by default #340
Conversation
Codecov Report
@@ Coverage Diff @@
## main #340 +/- ##
==========================================
- Coverage 60.02% 59.93% -0.10%
==========================================
Files 37 37
Lines 4025 4043 +18
==========================================
+ Hits 2416 2423 +7
- Misses 1402 1410 +8
- Partials 207 210 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
matevz
force-pushed
the
matevz/feature/drop-unencrypted-txes
branch
3 times, most recently
from
0ce4b60 to
8e0b48c
Compare
.github/workflows/ci-test.yaml
Outdated
| @@ -142,7 +142,9 @@ jobs: | |||
| run: tests/tools/spinup-oasis-stack.sh > /dev/null & | |||
|
|
|||
| - name: Unit tests with coverage | |||
| run: go test -race -coverpkg=./... -coverprofile=coverage.txt -covermode=atomic -v ./... | |||
| run: | | |||
| cp conf/tests-c10l.yml conf/tests.yml | |||
There was a problem hiding this comment.
Maybe it would be nicer to use the default test config (conf/tests.yml) and use ENV variables to set the necessary overrides.
| return common.Hash{}, ErrInvalidRequest | ||
| } | ||
|
|
||
| ethTx.Data() |
| @@ -463,6 +467,13 @@ func (api *publicAPI) SendRawTransaction(ctx context.Context, data hexutil.Bytes | |||
| return common.Hash{}, ErrMalformedTransaction | |||
| } | |||
|
|
|||
| if !api.checkOasisTxEncrypted(ethTx.Data()) { | |||
There was a problem hiding this comment.
Maybe rename the method to isEncrypted(tx) and only call it if allowUnencryptedTxs is set to false. Then it can also be made a standalone function (and not a method on the api backend), which will make it easy to write some unit tests for it.
It might also make sense to return a more descriptive error on rejection.
if !api.allowUnencryptedTxs && !isEncrypted(ethTx.Data()) {
logger.Debug("unencrypted transaction rejected", "hash" ...,
return common.Hash{}, ErrUnencryptedNotAllowed
} 
|
Hm, I'm not convinced this belongs in the gateway? Shouldn't this be up to the clients? |
matevz
force-pushed
the
matevz/feature/drop-unencrypted-txes
branch
from
8e0b48c to
ff64972
Compare
| return true | ||
| } | ||
|
|
||
| var tx types.Transaction |
There was a problem hiding this comment.
This does not seem correct as the data field does not contain an entire SDK transaction, but rather just a call? This should have a corresponding E2E test.
|
Re: discussion with Matevz Based on the hostname, could we force encrypted transaction? E.g. |
To put our discussion into context, since we still want to support standard Ethereum transactions, we cannot simply drop all unencrypted ones on the existing public RPC endpoint. But what we can do for c10l dApp developers out there that know they want to use explicitly encrypted txes and queries is to set up another Web3 endpoint that requires encrypted transactions, e.g. encrypted.sapphire.oasis.io. While this is fine for CLI dApps, where you simply point to a different RPC, it may cause some issues on the frontend MetaMask where the RPC URL should be unique per chain ID. |
This PR:
gateway.allow_unencrypted_txsflag to config which enables accepting unencrypted transactionsconf/tests-c10l.ymland use it in Sapphire e2e testsserver.ymlconfig