-
Notifications
You must be signed in to change notification settings - Fork 51
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
Adds mainnet archival rpc and fixes ref-finance example #57
Conversation
|
||
// type taken from near_primitives::hash::CryptoHash. | ||
/// CryptoHash is type for storing the hash of a specific block. | ||
pub struct CryptoHash(pub [u8; 32]); |
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 use newtype for this over just an alias of [u8; 32]
for simplicity?
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 just took whatever was in near_primitives
. It's a newtype there, so stuck with that. I don't know if changing it to an alias of [u8; 32]
would make it harder to transition later when near_primitives
is 1.0
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 would it be hard to transition? Just wrapping the bytes passed in with CryptoHash
internally? Am I missing something with what you mean 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.
I'm just thinking whether or not if we end up just exposing near_primitives::hash::CryptoHash
later and its still just a newtype which would make it a breaking change to transition over to. But maybe not a huge concern if we force everyone to just rely on our CryptoHash
from workspaces
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.
yeah, the primitives type prob shouldn't be tied to our API. I'm not sure if I'm misunderstanding what you're saying here though. Why would we ever need to expose the primitives type to be a part of our API?
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 was just thinking of this case, because there was a conversation we had about potentially exposing the near_primitives
/near_account_id
types once they released 1.0. But regardless of that, I think the newtype here would be better suited since it might be harder for users to create CryptoHash
es if it was just an alias of [u8; 32]
. The FromStr
impl should allow us to create it easily
I'll just merge this one in since this one has a fix for the other PRs stalling. If there's anymore concerns, we can just address them in a later ticket/PR |
This will fix the issue with #49. This was because ref-finance contract on mainnet on updated and so did some deposit requirements get upped. With this PR, to remedy the issue, mainnet archival rpc was added so we can grab the contract from a specific block height instead of the latest block height