-
Notifications
You must be signed in to change notification settings - Fork 93
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
Smart Contract Support #68
Conversation
Codecov Report
@@ Coverage Diff @@
## main #68 +/- ##
==========================================
+ Coverage 99.70% 99.73% +0.02%
==========================================
Files 55 67 +12
Lines 3409 3753 +344
==========================================
+ Hits 3399 3743 +344
Misses 10 10
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
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.
Hey, thanks.
I left some early comments, mainly to avoid redundancy with existing code (abi, units, utils).
One thing that bothered me in Ethereum.rb is that the Contract always contains a client. It should be the other way around; you have a client and pass a contract to deploy (just like transactions). What do you think?
In general, I would suggest writing tests first and building everything against the spec so that we know what does what and what functions how. A nice side effect would be 100% coverage 😁
lib/eth/contract.rb
Outdated
@name = name | ||
@code = code | ||
@abi = abi | ||
@client = client |
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.
This is a reverse dependency. A contract should not contain a client but rather a client should be able to deploy and wait.
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 plan to implement deploy_and_wait in client.
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 consider the following plan & interface.
before
@contract.deploy_and_wait
@contract.transact_and_wait.function_name(params)
@contract.call.function_name(params)
after
@client.deploy_and_wait(contract)
@client.transact_and_wait(contract, function_name, params)
@client.call(contract, function_name, params)
lib/eth/contract.rb
Outdated
end | ||
|
||
def address=(addr) | ||
@address = addr |
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.
address should always be an Eth::Address for security reasons.
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 now use Eth::Address 📝
lib/eth/contract.rb
Outdated
@deployment = Eth::Contract::Deployment.new(tx, @client) | ||
end | ||
|
||
def deploy_and_wait(*params, **args, &block) |
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.
All these functions should be implemented for the client not the contract. See transfer_and_wait.
lib/eth/contract.rb
Outdated
end | ||
|
||
def estimate(*params) | ||
result = @client.eth_estimate_gas(deploy_args(params)) |
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.
This or use Tx.estimate_intrinsic_gas().
lib/eth/contract.rb
Outdated
else | ||
tx = send_transaction(call_args(fun, args)) | ||
end | ||
return Ethereum::Transaction.new(tx, @client, call_payload(fun, args), args) |
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.
Eth::Tx?
lib/eth/contract/encoder.rb
Outdated
@@ -0,0 +1,115 @@ | |||
module Eth | |||
class Contract::Encoder | |||
def encode(type, value) |
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.
See Eth::Abi.encode()
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.
Removed dependency on Contract::Encoder using Eth::Abi.encode().
lib/eth/contract/formatter.rb
Outdated
module Eth | ||
class Contract::Formatter | ||
|
||
UNITS = { |
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.
See Eth::Unit::
lib/eth/contract/formatter.rb
Outdated
tether: 1000000000000000000000000000000 | ||
} | ||
|
||
def valid_address?(address_string) |
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.
Eth::Address.new(...).valid?
lib/eth/contract/formatter.rb
Outdated
return !(address.match(/[0-9a-fA-F]+/).nil?) | ||
end | ||
|
||
def from_bool(boolval) |
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.
Wondering what this is used for? Might want to live in Eth::Util
eth.gemspec
Outdated
@@ -48,4 +48,7 @@ Gem::Specification.new do |spec| | |||
|
|||
# scrypt for encrypted key derivation | |||
spec.add_dependency "scrypt", "~> 3.0" | |||
|
|||
# activesupport for smart contract support | |||
spec.add_dependency "activesupport", "~> 7.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.
where is this used?
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.
It was used in present?
and underscore
methods.
I will probably remove it later when it is no longer needed.
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 those are both one-line methods that could be copied and pasted into Eth::Util
or similar rather than pull in the entirety of activesupport
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.
Yes, that's what I was thinking. Keep dependencies as low as possible.
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.
Remove the dependency on activesupport and use the methods of Ruby itself.
Thank you for your comment. I will write a test and try to avoid redundancy 😄 |
Also, add yourself to the AUTHORS file :) |
Hi there, checking in on the status of this PR, would be very helpful to implement EIP-1271 for Sign-In with Ethereum spruceid/siwe-ruby#15, let me know if there is anything I can do to help! |
@w4ll3 Thank you. I'll call you when I need help! |
@q9f |
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.
This is beautiful, thank you for taking the time to merge it in!
lib/eth/contract.rb
Outdated
|
||
# -*- encoding : ascii-8bit -*- | ||
|
||
require "forwardable" |
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.
do we need this as dependency in the gemspec?
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.
Removed as not needed here.
lib/eth/client.rb
Outdated
# @param address [String] contract address. | ||
# @return [Object] returns the result of the call. | ||
def transact(contract, function_name, *args, **kwargs) | ||
gas_limit = Tx.estimate_intrinsic_gas(contract.bin) - Tx::DEFAULT_GAS_LIMIT + 53000 |
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.
just a tiny nitpick: let's create a Tx::CREATE_GAS = 32_000
constant that we can use 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.
OK, I'll try to fix it.
Co-authored-by: Afr Schoe <58883403+q9f@users.noreply.github.com>
Co-authored-by: Afr Schoe <58883403+q9f@users.noreply.github.com>
Co-authored-by: Afr Schoe <58883403+q9f@users.noreply.github.com>
…rt-contract-support
@q9f I have corrected the points you just reviewed! |
Epic! |
I plan to create a smart contract feature that will allow for the following.
I am currently building: