-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DO NOT MERGE] Create backend web server implementation with rust framework #37
base: offramp-prototype-staging
Are you sure you want to change the base?
[DO NOT MERGE] Create backend web server implementation with rust framework #37
Conversation
|
Name | Link |
---|---|
🔨 Latest commit | 8fd04eb |
@ebma @bogdanS98 @gianfra-t Initial review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I really like more the rust version of the back-end. It's very neat @b-yap 👍.
I just left a few comments but not critical. The only thing is that I couldn't run this on my pc, I get Library not loaded: @rpath/libpq.5.dylib
. Do I need to install postgres maybe?
let mut tx = create_transaction_no_operations( | ||
funding_account.public_key(), | ||
// increment the sequence number | ||
sequence + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we increase the sequence by one here? In the js backend we didn't need to.
Is it because new TransactionBuilder
on the js library will increase by 1 internally while substrate-stellar-sdk
doesn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this answer my question 😅. You can ignore this one.
|
||
match memo_type.as_str() { | ||
"1" | "text" | "memotext" => { | ||
let memo = self.memo.clone().into_bytes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to use the Memo
implementation to build from string here to simplify this conversion a bit? WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
let _memo = self.memo.as_bytes(); | ||
let mut memo = [0;32]; | ||
memo[.._memo.len()].copy_from_slice(_memo); | ||
Ok(Memo::MemoHash(memo)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thought as the above with from_hash_memo, although I'm not sure if IntoHash
is implemented for [u8]
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, IntoHash
is not implemented for &[u8]
and Vec<u8>
.
It can accept Hash
or [u8;32]
, which is already the type of the Enum tuple:
pub enum Memo {
...
MemoHash(Hash),
....
}
But your comment helped discover a problem with the previous code. Decoding from base64 is actually required (which I didn't do):
transactionMemo = Memo.hash(Buffer.from(memo, "base64"));
return failed_status; | ||
}; | ||
|
||
if native_balance.balance < 2.5 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can use a multiple of NEW_ACCOUNT_STARTING_BALANCE
defined here since this account will always need at least a bit more than that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool implementation 👍 We of course don't need the database yet (and probably wouldn't use it to store the token info) but it's nice to already see how easy it is to deal with the schemas with diesel.
I would suggest we keep the code around and switch from the Typescript/express backend to this one either once we identify the need to use databases in the prototype, or when we reach a state where we want move away from the prototype and aim for a more production-ready/stable backend.
and update memohash
01d9d7b
to
8fd04eb
Compare
@gianfra-t Yes you will need postgres for this. I have updated the README to either install it in your machine, or use docker. |
I also added a note on how to install it via brew to the README. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great implementation @b-yap 🚀! I only have one question.
Is the infra
folder supposed to contain anything else than the models, schemas and db interaction code in the future? If not, then I think we could rename it to something like db
so that it's a bit clearer.
addresses #31
command:
RUST_LOG=info cargo run
, and it will show:Looks like the provided secret key doesn't work on PUBLIC network, so currently I used TESTNET.