Prototype of Ethereum - Stellar inverse transaction flow #50
Conversation
| panic(err) | ||
| } | ||
|
|
||
| w.WriteHeader(http.StatusAccepted) |
There was a problem hiding this comment.
❓ I note that generating a signature seems to be an async operation, where I think it could probably be a synchronous operation. What makes it needs to be async?
If we make it synchronous, then there wouldn't be a need for a background worker to be running inside this same process.
There was a problem hiding this comment.
I just updated the PR description with reasoning behind signature requests.
| } | ||
|
|
||
| if s.kp == nil { | ||
| s.kp = keypair.MustParseFull(s.SecretKey) |
There was a problem hiding this comment.
💡 You should move this into the Signer, make it a *keypair.Full, so that you know at the time of setting up the Signer whether it is valid or not, and then you don't have to have panics here in the code on an invalid value.
| sourceAccount, err := client.AccountDetail(horizonclient.AccountRequest{AccountID: txSource}) | ||
| if err != nil { | ||
| return xdr.TransactionEnvelope{}, errors.Wrap(err, "error getting account details") | ||
| } |
There was a problem hiding this comment.
💡 It looks like the only reason we need to hit horizon during this request is to get the sequence number of the txSource, however we can offload that responsibility to the caller by having them provide the seq number. It would reduce the work that the bridge has to do, and make it harder for someone to use a bridge validator to DOS a horizon.
There was a problem hiding this comment.
Good idea, I added this for simplicity of this prototype but I was wondering if we could leave it like this. However, providing a seqnum loaded by a client is a better option.
There was a problem hiding this comment.
I think it would be better for the client to pick the seq number as well. If the user has control over sequence numbers it will reduce the cases where transactions fail due to the sequence number conflicting with another transaction belonging to the user
| log.Fatal("Unable to access Horizon root resource") | ||
| } | ||
|
|
||
| ledgerSequence := uint32(root.HorizonSequence) |
There was a problem hiding this comment.
I think we want to be fetching / recording the last ledger we ingested in the db
| TransactionBlob string | ||
| } | ||
|
|
||
| func (m *Memory) GetIncomingEthereumTransactionByHash(hash string) (IncomingEthereumTransaction, error) { |
There was a problem hiding this comment.
We should not need to ingest ethereum transactions. So I think it makes more sense to include this function in a struct representing the ethereum client instead of the Memory store
| defer m.mutex.Unlock() | ||
|
|
||
| if len(m.signatureRequests) == 0 { | ||
| return nil, sql.ErrNoRows |
There was a problem hiding this comment.
nit: I don't think it's necessary to return sql.ErrNoRows because the returned slice will be empty anyways
| } | ||
| } | ||
|
|
||
| return SignatureRequest{}, sql.ErrNoRows |
There was a problem hiding this comment.
nit: I think it would be better to have an error defined in this package because sql.ErrNoRows exposes implementation details
|
Can I have 👍 on this PR. Most of the suggestions here will be implemented when I move the DB backend do use Postgres. |
This commit adds initial versions of different services of Starbridge server. In general the server is currenly divided into two parts: frontend and backend. Frontend handles HTTP requests and allows users to create
SignatureRequest's (requests to sign an inverse transaction) and send it to a backend server via a queue (or a DB). Backend server is responsible for streaming blockchain data, handlingSignatureRequest's and finally creating and signing transactions. This separation was made to allow more secure architectures in which frontend server does not have access to signing part of the app.In the backend server, transaction builder and transaction signer are separate entities. This is done to (maybe) support using secure enclaves (like AWS Nitro Enclaves) that will store the secret key and sign transactions.
List of services:
backend/Worker- Main backend worker handlingSignatureRequeststellar/controllers/StellarGetInverseTransactionForEthereum- Controller responsible for building inverse transactions, contains simple logic for checking if a newSignatureRequestshould be created or if the inverse transaction should be returned to the userstellar/signer/Signer- Stellar transaction signerstellar/txbuilder/Builder- Stellar transaction builder, currently ETH tx hardcodedstellar/txobserver/Observer- Observer of Stellar transactions, currently using testnet.store/Memory- Storage system implemented in memory, in the future: Postgres DB.The current version contains at least one race condition because
UpsertOutgoingStellarTransactionis called concurrently from two different services. This should be solved bySELECT ... FOR UPDATEin Postgres but I wonder if we should allow just one DB transaction at a time for security reasons (to decrease number of synchronization issues).