Conversation
9bc04d4 to
d4a9047
Compare
gilcu3
left a comment
There was a problem hiding this comment.
Looks good overall, although there are three points that need improvements:
- There were some changes in the struct, that should be reflected in the doc
- We are serde serializing a bunch of structs as arrays, but should be hex to make them human readable and compact
- We are duplicating type names in the contract ABI interface, which IMO we should avoid
51a1685 to
f3e89ea
Compare
| pub struct BitcoinRpcRequest { | ||
| pub tx_id: BitcoinTxId, // This is the payload we're signing | ||
| pub confirmations: usize, // required confirmations before considering final | ||
| pub confirmations: u64, // required confirmations before considering final |
There was a problem hiding this comment.
What are the values of this?
I am wondering whether it is not possible to do generalize the implementation, something like this instead:
struct RpcRequest<T: TxId, Extractor, Confirmation>{
pub tx_id: T
pub confirmation: Confirmation,
pub extractors: Vec<Extractor>
}
trait TxId{
...
}
...
pub struct BitcoinRpcRequest = RpcRequest<Confirmation, BiitcoinExtractor, u64>
pub struct SolanaRpcRequest = RpcRequest<Finality, SolanaExtractor, Finality>
I guess you understood what kind of generalization I am talking about?
There was a problem hiding this comment.
This might be generalizable, but given the few number of initial chains I'd advocate for starting specific. For example we might want to expose both number of confirmations and some finality notion for a single chain (as Ethereum has both notions). The above structure would make that hard.
Once we have a bunch of chains supported we could break out the shared logic to something like the above if we think it would simplify things.
| pub chain: ForeignChain, | ||
| pub tx_id: EvmTxId, |
There was a problem hiding this comment.
Nit: For the sake of order, please swap those lines
There was a problem hiding this comment.
Can you add more context, why should tx_id be before the chain specification?
There was a problem hiding this comment.
Because in the other structs, tx_id is first, just to keep better readibility
closes #1921