[NONEVM-706][Solana][deprecated] - Refactor TXM + Rebroadcast Expired Tx functionality #928
Conversation
pkg/solana/txm/txm.go
Outdated
| // Otherwise, it marks the transaction as errored. | ||
| func (txm *Txm) handleErrorSignatureStatus(sig solanaGo.Signature, status *rpc.SignatureStatusesResult) { | ||
| // We want to rebroadcast rather than drop tx if expiration rebroadcast is enabled when blockhash was not found. | ||
| if status.Err != nil && status.Err == client.ErrBlockhashNotFound && txm.cfg.TxExpirationRebroadcast() { |
There was a problem hiding this comment.
Huh I didn't realize we had these error patterns already defined in the client package. But don't think we can just do an == here for this match. The status.Err interface doesn't return like a normal RPC string. Think it looks something like this so don't think we can use these patterns. We probably have to do something like this then a strings.Contains
pkg/solana/txm/txm_internal_test.go
Outdated
| mc.On("LatestBlockhash", mock.Anything).Return(&rpc.GetLatestBlockhashResult{ | ||
| Value: &rpc.LatestBlockhashResult{ | ||
| LastValidBlockHeight: 100, | ||
| Blockhash: solana.Hash{}, | ||
| }, | ||
| }, nil).Once() |
There was a problem hiding this comment.
Looks like we mock the LatestBlockhash the same way for all of the tests. Could we move this one layer higher so we only have to mock it once? We can ditch the .Once() unless some tests don't get that far then I'm ok with replacing it with .Maybe()
There was a problem hiding this comment.
ohh yes removing the once makes that generalization work 👍
pkg/solana/txm/txm_internal_test.go
Outdated
| @@ -908,6 +983,12 @@ func TestTxm_compute_unit_limit_estimation(t *testing.T) { | |||
| ) | |||
|
|
|||
| t.Run("simulation_succeeds", func(t *testing.T) { | |||
| mc.On("LatestBlockhash", mock.Anything).Return(&rpc.GetLatestBlockhashResult{ | |||
pkg/solana/txm/txm_internal_test.go
Outdated
| cfg := config.NewDefault() | ||
| cfg.Chain.FeeEstimatorMode = &estimator | ||
| cfg.Chain.TxConfirmTimeout = relayconfig.MustNewDuration(5 * time.Second) | ||
| cfg.Chain.TxRetentionTimeout = relayconfig.MustNewDuration(10 * time.Second) // Enable retention to keep transactions after finality and be able to check. |
There was a problem hiding this comment.
nit: think this comment is maybe incomplete?
| cfg.Chain.TxRetentionTimeout = relayconfig.MustNewDuration(10 * time.Second) // Enable retention to keep transactions after finality and be able to check. | |
| cfg.Chain.TxRetentionTimeout = relayconfig.MustNewDuration(10 * time.Second) // Enable retention to keep transactions after finality and be able to check their statuses. |
pkg/solana/txm/txm_internal_test.go
Outdated
| txID := "test-rebroadcast" | ||
| assert.NoError(t, txm.Enqueue(ctx, t.Name(), tx, &txID)) | ||
| wg.Wait() | ||
| time.Sleep(2 * time.Second) // Sleep to allow for rebroadcasting |
There was a problem hiding this comment.
You don't have to use this if 2 second sleep is deterministic so rebroadcast always happen in that time. If not, maybe it's safer to use something like waitFor(t, waitDuration, txm, prom, empty) which we use in other tests above to wait till all tx have gone through.
pkg/solana/txm/txm_internal_test.go
Outdated
| slotHeightFunc := func() (uint64, error) { | ||
| return uint64(1500), nil | ||
| } | ||
| // Mock LatestBlockhash to return a invalid blockhash in the first 3 attempts (initial + 2 rebroadcasts) |
There was a problem hiding this comment.
nit: typo
| // Mock LatestBlockhash to return a invalid blockhash in the first 3 attempts (initial + 2 rebroadcasts) | |
| // Mock LatestBlockhash to return an invalid blockhash in the first 3 attempts (initial + 2 rebroadcasts) |
pkg/solana/txm/txm_race_test.go
Outdated
| "github.com/gagliardetto/solana-go" | ||
| solanaGo "github.com/gagliardetto/solana-go" |
There was a problem hiding this comment.
Looks like we have duplicate imports here
pkg/solana/txm/txm_race_test.go
Outdated
| @@ -84,7 +85,12 @@ func TestTxm_SendWithRetry_Race(t *testing.T) { | |||
|
|
|||
| t.Run("delay in rebroadcasting tx", func(t *testing.T) { | |||
| client := clientmocks.NewReaderWriter(t) | |||
| // client mock | |||
| client.On("LatestBlockhash", mock.Anything).Return(&rpc.GetLatestBlockhashResult{ | |||
There was a problem hiding this comment.
Like in the other tests, I think we can move this mock to the top level since we return the same thing for all of them
Farber98
force-pushed
the
nonevm-706-support-custom-bumping-strategy-rpc-expiration-within-confirmation
branch
from
9f5ded1 to
a45fa1e
Compare
lastValidBlockHeight shouldn't be exported better comment refactor sendWithRetry to make it clearer confirm loop refactor move accountID inside msg lint fix base58 does not contain lower l fix hash errors fix generate random hash remove blockhash as we only need block height expired tx changes without tests add maybe to mocks expiration tests send txes through queue revert pendingtx leakage of information. overwrite blockhash fix order of confirm loop and not found signature check fix mocks fix test fix pointer add comments reduce rpc calls + refactors tests + check to save rpc calls address feedback + remove redundant impl iface comment address feedback on compute unit limit and lastValidBlockHeight assignment blockhash assignment inside txm.sendWithRetry address feedback
…y-rpc-expiration-within-confirmation
Farber98
force-pushed
the
nonevm-706-support-custom-bumping-strategy-rpc-expiration-within-confirmation
branch
from
1fc7ee7 to
19336a5
Compare
Farber98
had a problem deploying
to
integration
GitHub Actions
Failure
|
Farber98
changed the title
Description
Refactor
confirmandsendWithRetryfuncsAdding a new
TxExpirationRebroadcasttoggable routine:confirmloop, after checking confirmations, we will determine if we need to rebroadcast expired transactions that where not processed yet. Rebroadcasting a transaction involves:sendWithRetrydirectly without enqueuinghttps://smartcontract-it.atlassian.net/browse/NONEVM-706