feat: global contract signing support #29
Conversation
frol
left a comment
frol
left a comment
There was a problem hiding this comment.
There are no new screenshots, so I assume that there are not enough tests added
| for _i in 0..(chunks as usize) { | ||
| stream.read_exact(&mut buf)?; | ||
| } | ||
| stream.read_exact(&mut buf[0..(remainder as usize)])?; |
There was a problem hiding this comment.
Does it work well if the remainder is 0 and thus stream.read_exact(buf[0..0])?
There was a problem hiding this comment.
Yup. For instance, if we transfer 100 character long contract (making remainder 0), using near-cli-rs, the hash would be the same both on ledger and cli for checking.
There was a problem hiding this comment.
Also, a read function implementation for io::Read trait always tries to check if there is something in buffer, so hash shouldn't be impacted (src/parsing/transaction_stream_reader/mod.rs):
impl<R: io::Read> io::Read for HashingStream<R> {
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
if !buf.is_empty() {
let n = self.reader.read(buf)?;
// update hash on each chunk passing through
if n > 0 {
let data = &buf[0..n];
self.sha256
.update(data)
.map_err(|_err| io::Error::from(io::ErrorKind::OutOfMemory))?;
}
return Ok(n);
}
Ok(0)
}
}| GlobalContractDeployMode::CodeHash => "Code hash (immutable)", | ||
| GlobalContractDeployMode::AccountId => "Account Id (mutable)", |
There was a problem hiding this comment.
Since it is not a menu of items that users will be able to select, and they will only see one or the other, I believe it can be confusing:
Deploy mode
Code hash (immutable)
As a user I start to ask myself:
- What is "immutable"? It is quite a niche term and yet not well-defined in this context
- Does "immutable" mean that I won't ever be able to deploy another contract to the account? (not true - another contract can be deployed to the account)
- Are there "mutable" contracts? How to mutate them?
Deploy mode
Account Id (mutable)
- Aren't all contracts deployed to some account id?
- How "mutable" is the deployment?
I suggest to change it to one of these two options (or suggest something better based on how it looks on the screen):
Option 1:
"Can Be Used"
"by Code Hash"
"by Account ID (auto-upgrade everywhere it is used)"
Option 2:
"Deploy Mode"
"New global contract that can be used by contract hash"
"Auto-update all accounts that use the global contract by the Account ID"
There was a problem hiding this comment.
My logic was to use the annotations/strings from near-cli-rs, so people could reference that and double-check themselves. Despite this, I did find it confusing as well when trying to sign it like that :|
Hence, option 1 makes the most sense to me - concise + can see it in 1 screen. Like: Action? Global Contract -> Hash for contract? Here is SHA256 -> How can/will it be used? by code hash or account id
There was a problem hiding this comment.
@vsavchyn-dev We will need to update near-cli-rs messaging as well. It is not set in stone
frol
left a comment
frol
left a comment
There was a problem hiding this comment.
@vsavchyn-dev Thank you for quickly addressing my review comments. I believe this PR is in a good shape to be reviewed by the Ledger team
|
@tdejoigny-ledger @fbeutin-ledger This PR is ready for your review |
|
@frol it seems that there are still some failures in the CI and then a sec audit will be required. |
|
@tdejoigny-ledger I've updated both
I've tested this extensively using Speculos emulation, test helper that we have, and reusable workflow script that was failing ( The problem appears to be isolated to either:
Are there any known fixes to this issue or is there anything else I can do to resolve this issue? |
|
@yogh333 Do you have any idea what the problem is here? |
I will have a look |
|
@vsavchyn-dev @frol Hi I have fixed the issue with this PR: #30 |
|
@tdejoigny-ledger checks for nanosp and nanox pass now. You also mentioned something about security audit, right? |
|
@vsavchyn-dev yes good news ! the CI is 🟢 |
|
@tdejoigny-ledger are you referring to a full scale security audit? This sounds like an overkill for the change that just adds two new types of transaction action. Can you just take my review as attestation that this change matches the new protocol feature and implements it fully as in spec? |
hello @frol as I said a security audit needs to be conducted for these changes. |
|
@tdejoigny-ledger we don't have the capacity to conduct the audit, so we will leave it here as is in case you will ever have the capacity to review it. |
Hello @frol |
|
@tdejoigny-ledger I'd like to see the confirmation that all these changes received the audit: 
Some of them did not have a single peer review, others do not even pass your CI, and the ones that add new functionality (#12, #17) did not have a single test case added. I thought you either follow the process always, or make the exceptions for quality contributions - we added extensive testing, did peer reviews, and only involved you after we got extensive testing on our side. Security audit is just another human reading the code. Be that human for this PR. |
|
@tdejoigny-ledger Regarding the audit, the bridge and other teams from Near eco can allocate resources if needed to review these changes. From my perspective these reviews will be more valuable than side audit by auditors who aren't familiar with the ecosystem. |
Hello @karim-en , it’s important to distinguish a functional review, which is always welcome and essential; from the security review, which focuses on security vulnerabilities/weaknesses and is mandatory to publish an app on our LW store. |
|
@tdejoigny-ledger I assume you have an ongoing engagement with some audit company, can you ask them to audit this PR just like you do for other PRs? |
5 participants
Checklist
develop