Skip to content

[libra-dev] Get Serialized Signed Transaction - #9

Merged
sunmilee merged 1 commit into
masterfrom
txn
Nov 20, 2019
Merged

[libra-dev] Get Serialized Signed Transaction#9
sunmilee merged 1 commit into
masterfrom
txn

Conversation

@sunmilee

Copy link
Copy Markdown
Contributor

This PR provides an API to get a serialized SignedTransaction type given the transaction metadata and a private key in bytes.

@thefallentree

Copy link
Copy Markdown

I think you didn’t ran bindgen

Comment threadlibra-dev/include/data.h Outdated
struct CDevAccountResource account_resource_from_lcs(const uint8_t *buf, size_t len);
void account_resource_free(struct CDevAccountResource *point);

uint8_t* lcs_signed_transaction(uint64_t sender[32], uint64_t receiver[32], uint64_t sequence, uint64_t num_coins, uint64_t max_gas_amount, uint64_t gas_unit_price, uint64_t expiration_time_millis, uint8_t* private_key_bytes);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't just return an pointer , the user will have no means to free the memory. Instead we need to let user pass in an pre-allocated buffer pointer, with size, and we have to return an error if size is too small

Comment threadlibra-dev/src/transaction.rs
Comment threadlibra-dev/include/data.h Outdated
void account_resource_free(struct CDevAccountResource *point);

void lcs_signed_transaction(const uint8_t sender[32], const uint8_t receiver[32], uint64_t sequence, uint64_t num_coins, uint64_t max_gas_amount, uint64_t gas_unit_price, uint64_t expiration_time_millis, const uint8_t* private_key_bytes, uint8_t** buf, size_t* len);
void free_txn(uint8_t** buf);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should name it as
libra_signed_transcation_build() and libra_signed_transcation_free() for consistency (I'm doing it for AccountResource too)

Comment threadlibra-dev/src/account_resource.rs Outdated
987654321,
123456789,
ByteArray::new(vec![1,2,3,4]),
ByteArray::new(vec![1, 2, 3, 4]),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to rebase

Comment threadlibra-dev/src/transaction.rs
Comment threadlibra-dev/src/transaction.rs Outdated

// generate key pair
let mut seed: [u8; 32] = [0u8; 32];
seed[..4].copy_from_slice(&[1, 2, 3, 4]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused seed?

@thefallentreethefallentree left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

Comment threadlibra-dev/Cargo.toml Outdated
[dev-dependencies]
transaction-builder = {git = "https://github.com/libra/libra.git", branch = "testnet" }
lcs = { git = "https://github.com/libra/libra.git", branch = "testnet", package = "libra-canonical-serialization" }
rand = "0.6.5"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this is only used in test ? so it should be in dev-dependencies

Comment threadlibra-dev/include/data.h Outdated
struct CDevAccountResource account_resource_from_lcs(const uint8_t *buf, size_t len);
void account_resource_free(struct CDevAccountResource *point);

/// To get the serialized transaction in a memory safe manner, the client needs to pass in a pointer to a pointer to the allocated memory in rust

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use doxgen tags (all tools will be able to use it, including automatically generating html docs)
https://www.rosettacommons.org/docs/latest/development_documentation/tutorials/doxygen-tips#common-doxygen-tags

specifically, mark the params with @params[in] @params[in/out] etc will be very useful.

Also, you need to say user take ownership of pointer returned by *buf, which needs to be freeed by libra_signed_transcation_free

Comment threadlibra-dev/src/data.rs Outdated
)
);
}
extern "C" {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does these actually works other than just generate warnings? Myabe we should just put the data types in data.rs instead.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could do that and not generate functions

Comment threadlibra-dev/src/lib.rs Outdated

pub mod account_resource;

mod transaction;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pub mod ?

Comment threadlibra-dev/src/transaction.rs Outdated
buf: *mut *mut u8,
len: *mut usize,
) {
let sender_buf: &[u8] = unsafe { slice::from_raw_parts(sender, ADDRESS_LENGTH) };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for &[u8] I think, rust will figure it out.

Comment threadlibra-dev/src/transaction.rs
Comment threadlibra-dev/src/transaction.rs
Comment threadlibra-dev/src/transaction.rs Outdated

#[no_mangle]
pub unsafe extern "C" fn libra_signed_transaction_free(buf: *mut *mut u8) {
libc::free(*buf as *mut libc::c_void);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should check for NULL

@thefallentreethefallentree left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some small changes

@sunmilee
sunmilee merged commit ca874c7 into masterNov 20, 2019
@thefallentree
thefallentree deleted the txn branch December 12, 2019 21:23
thefallentree pushed a commit that referenced this pull request Sep 22, 2020
upgrade to latest testnet, fix account role missing fields
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@sunmilee@thefallentree