As/full system test#51
Conversation
…system test flows
… add WalletDriver.AddToken for ERC-20 token registration in tests
…ndshake and cross-restart state survival
…es when on-chain TEE signer mismatches the executor's real signing key
…h fullstack tests (catches silent retry-and-recover regressions)
alsala
requested review from
andreanistico,
paolocappelletti and
saratnt
and removed request for
saratnt
saratnt
left a comment
saratnt
left a comment
There was a problem hiding this comment.
Only some minor comments
| //create client | ||
| testHelper := pestestutil.NewSimTestHelper(t, true, true, nil, teeKey.PublicKey().Bytes()) | ||
| autoMining, useMockContracts := true, true | ||
| testHelper := pestestutil.NewSimTestHelper(t, autoMining, useMockContracts, nil, teeKey.PublicKey().Bytes()) |
There was a problem hiding this comment.
Considering that every where NewSimTestHelper is called the only param that changes is the last one, that can be nil or teeKey.PublicKey().Bytes(), consider creating an utility function like this:
func SetupDefaultTestHelper(pubKey []byte) *pestestutil.SimTestHelper {
autoMining, useMockContracts := true, true
return pestestutil.NewSimTestHelper(t, autoMining, useMockContracts, nil, pubKey)
}
| @@ -25,7 +25,8 @@ func TestGetPendingPaymentsCmd(t *testing.T) { | |||
| key1, err := crypto.GeneratePrivateKeySecp256k1() | |||
| require.NoError(t, err) | |||
|
|
|||
There was a problem hiding this comment.
getpendingpayments.go wasn't refactored with the Exec function, everything is still in Run
There was a problem hiding this comment.
You are right, the Exec refactor was driven by WalletDriver, every other command got it because some fullstack test needed to drive that command through the wallet path. No fullstack test had needed to drive getpendingpayments yet, so it was missed. I've refactored it, added the corresponding WalletDriver.GetPendingPayments wrapper, and written a new fullstack test that exercises both.
| // error string. | ||
| _, err = driver.RequestReport(t.Context(), "balances", "100 wei") | ||
| require.Error(t, err, "deanonymize request from unauthorized account must fail at submitRequest time") | ||
| require.True(t, |
There was a problem hiding this comment.
here and in other lines, you can use require.Contains (https://pkg.go.dev/github.com/stretchr/testify/require#Contains)
| var ETH_TOKEN = ethCommon.Address{} | ||
| // ETH_TOKEN is the package-local alias for the canonical native-token sentinel | ||
| // in vela-common-go. Matches Structs.sol: address constant ETH_TOKEN = address(0). | ||
| var ETH_TOKEN = velacommon.ETH_TOKEN |
There was a problem hiding this comment.
maybe it should be const instead of var?
There was a problem hiding this comment.
ethCommon.Address is [20]byte, and Go does not allow const arrays. A func ETH_TOKEN() would give true immutability, but it forces ETH_TOKEN() at every call site so I prefer to keep it as it is
| // Step 4: snapshot the user's on-chain balance before the claim. After | ||
| // the claim we expect this to increase by exactly refundTotal (deployer | ||
| // pays the gas in step 5). | ||
| ctx := context.Background() |
There was a problem hiding this comment.
you can use t.Context() as in other points
| // refundAmount and the contract crediting pendingClaims. | ||
| sim := suite.GetSimTestHelper() | ||
| ethAddr := ethCommon.Address{} | ||
| ethAddr := velacommon.ETH_TOKEN |
There was a problem hiding this comment.
Why in vela it is used NativeTokenAddress() and in vela-nova ETH_TOKEN?
There was a problem hiding this comment.
You are right, we should uniform the usage. There are pros and cons on using either, I finally opted for using ETH_TOKEN, same name as in sol contract.
3 participants
Adds the
WalletDrivertest harness and 15 fullstack tests that drive the full production path through the wallet CLI layer against the vela fullstack suite.wallet/testutil.WalletDriver: per-test wallet.conf + user-signed chain client per command; wrappers forDeployApp,RegisterUser,Deposit,Withdraw,PrivateTransfer,GetPrivateBalance,ClaimPendingPayments,RequestReport,DownloadReport,AddToken,SetApplicationID;NewWalletDriverNoDeployerRolevariant for access-control tests.Exec(ctx) erroron 8 commands (deposit, withdraw, privatetransfer, requestreport, downloadreport, registeruser, claim, getpublicbalance);parseAssetAmount/resolveContexthelpers;
getpublicbalancenow has an injectableClient, real ERC-20balanceOf, hermetic test (replaces the livebase-sepolia.drpc.orgcall); sharedruntime/wasm-go/testhelpersde-dupes WASM buildhelpers across test folders.
claim) + negative/security/robustness (unlisted-token revert,
DEPLOYER_ROLEenforcement, non-managerstateUpdate, TEE attestation rejection, invalidprevStateRoot, unauthorized authority deanonymize,private-transfer to unregistered recipient, multi-token/non-18-decimals isolation).
system_tests/payment_app_system_test.gofix for the zero-subtype path.Note: Currently refers to local
velarepo on branchas/full_system_test. CI is not passing as of now, until a tag is used.