fix: get local testing working, fix memory leak #89
Conversation
| rpc.CommitmentConfirmed, | ||
| ) | ||
|
|
||
| if err != nil { | ||
| c.lggr.Errorf("error on Transmit.SendAndConfirmTransaction: %s", err.Error()) | ||
| } | ||
| // TODO: poll rpc for tx confirmation (WS connection unreliable) |
There was a problem hiding this comment.
Was there an agreement we are dropping WS support for the long term?
I'd prefer we actually have two implementations, both simpler but more resource-intensive RPC polling, and WS subscription so we can make incremental improvements to it and potentially switch again once stable enough.
There was a problem hiding this comment.
Sort of, there was consensus for dropping it for now.
I agree that the RPC polling is more resource intensive, but the WS implementation in the solana-go module was the cause of a memory leak which would take down the core node. Building a stable WS implementation would take a lot of time and effort, so it was shelved for now.
There was a problem hiding this comment.
I'd just suggest we declare a TxConfirmer as an interface and then we implement the RPC TxConfirmer + some docs about what were the latest encountered issues using the WS TxConfirmer (no need to do any work on it now). That way we have a defined path of switching implementations if/when WS implementation becomes stable.
There was a problem hiding this comment.
that sounds like a good idea, but probably better addressed in a separate PR?
it will also likely tie into the tx management work for terra as well
There was a problem hiding this comment.
Yup, no need to slow down this PR whose main purpose is to quickly fix the memory leak (get the network stable).
I'll open a GH Issue to track this suggestion for the future.
And yeah, we should define types like TxCofirmer on the Relay SDK level and use (implement) them for specific relays.
There was a problem hiding this comment.
might be easier to add comments to this open issue for adding a tx confirmer: #90
Main: fix memory leak by removing WS connection
Secondary: small doc updates + remove extra modulus in contract
Tested locally for 2+ hours: ✅ contract fix, ✅ removed WS, ✅ memory leak
corresponding core implementation: #89