wip:add write capability#654
Conversation
Krish-vemula
force-pushed
the
feat/stellar-write-capability
branch
from
4196e56 to
053c462
Compare
… chainlink-common
Resolve conflicts between WriteReport wiring and Stellar monitoring/beholder metrics.
product-security-plaid-production
Bot
requested review from
Unheilbar and
fernandezlautaro
ilija42
left a comment
ilija42
left a comment
There was a problem hiding this comment.
Still reviewing, but there are missing features here that need to be implemented, so I will flag that in this initial review so that you can continue working on this
…early-return tx hash consensus
ilija42
left a comment
ilija42
left a comment
There was a problem hiding this comment.
Build is failing and test CI is red
|
| args, err := buildTransmissionInfoArgs(receiver, workflowExecutionID, reportID) | ||
| if err != nil { | ||
| return TransmissionInfo{}, err | ||
| } | ||
|
|
||
| resp, err := fc.SimulateTransaction(ctx, stellartypes.SimulateTransactionRequest{ |
There was a problem hiding this comment.
Take a look at the already established pattern with the EVM forwarder codec. Handling all the encoding/decoding in one place helps us isolate the concerns, ideally you always want to have stable APIs behind cohesive abstractions, this makes the code easier to understand and simpler to test.
| receiver string, | ||
| workflowExecutionID [32]byte, | ||
| reportID [2]byte, |
There was a problem hiding this comment.
Merge this into a TransmissionID struct, this makes enriching logs simpler as well as makes the code easier to understand
| return nil, err | ||
| } | ||
|
|
||
| rawReportVal := stellartypes.ScVal{Type: stellartypes.ScValTypeBytes, Bytes: report.GetRawReport()} |
There was a problem hiding this comment.
are nil and empty slice treated the same, do these args need to be sanitized?
| return info, nil | ||
| } | ||
|
|
||
| func parseFieldsIntoTransmissionInfo(info *TransmissionInfo, sv xdr.ScVal) { |
There was a problem hiding this comment.
This decoding should be stricter, we know exactly what the contract returns, this almost reads like a generic parse, instead we want light weight decoding so that we don't even need to rely on a generated contract client
| func observedWriteReportResult(reply *stellarcap.WriteReportReply) writeReportExecuteResult { | ||
| return writeReportExecuteResult{response: reply, billNode: false} | ||
| } | ||
|
|
||
| func submittedWriteReportResult(reply *stellarcap.WriteReportReply) writeReportExecuteResult { | ||
| return writeReportExecuteResult{response: reply, billNode: true} | ||
| } |
There was a problem hiding this comment.
These helpers are not needed if we just return billing metadata from the execute itself, its a bit more straightforward and follows the established pattern
| transmissionScheduler ts.TransmissionScheduler | ||
| } | ||
|
|
||
| type writeReportExecuteResult struct { |
There was a problem hiding this comment.
Lets follow the established evm pattern of how the reply is built, I'm finding it a bit hard to follow it here, no need to delegate billing decision
| reportID, | ||
| ) | ||
|
|
||
| info, err := wr.pollTransmissionInfo(ctx, request.ContractId, workflowExecutionID, reportID, queuePosition) |
There was a problem hiding this comment.
There might be an optimisation here if we simulate the tx and see that it fails we just skip the polling too. There are some edge cases here though, what if the simulation fails only on some of the node and how do we ensure same reply across the DON? I wouldn't block this PR because of it, but at least add a TODO and lets think about this as a followup
| switch postInfo.State { | ||
| case TransmissionStateSucceeded: | ||
| reply := wr.successReplyFromObservedState(postInfo) | ||
| populateReplyFromSubmit(reply, submitResp) |
There was a problem hiding this comment.
If Node1 submits TX, and Node2 sends duplicate TX (because of lagging RPC for instance), Node2's TX will revert onchain, however with populateReplyFromSubmit we will return Node2's TX hash.
EVM Handles it by GetSuccessfulTransmissionHash logic, which retrieves the transaction that actually delivered successful transmission, and its proven to be a useful guard from returning non-identical responses.
Is there any Stellar-related quirks that is stopping us from reusing EVM (and Solana) logic of populating reply with successful transaction hash?
| reply := wr.revertedReplyFromObservedState(postInfo, "receiver contract execution failed") | ||
| populateReplyFromSubmit(reply, submitResp) | ||
| return submittedWriteReportResult(reply), nil | ||
| default: |
There was a problem hiding this comment.
Should we handle unkown status and return error instead of silent fallback (same as EVM/Aptos/Solana)?
| return submittedWriteReportResult(reply), nil | ||
| case TransmissionStateInvalidReceiver: | ||
| reply := wr.revertedReplyFromObservedState(postInfo, "receiver contract cannot accept reports: not a Wasm contract or missing on_report function") | ||
| populateReplyFromSubmit(reply, submitResp) |
There was a problem hiding this comment.
Same here, we need to ensure we returned the same TxHash Across all the nodes like EVM/Solana do
3 participants
No description provided.